Skip to content

Improving clarity and error handling around absent values #360

Open
@warpfork

Description

As of v0.14ish (and considerably before), the exact contract that the datamodel.Node interface has for values that are not found in a map (or are out of bounds index in a list) is not super well documented, nor is a defacto convention well adhered-to in implementations. This needs to be rectified. The current situation makes writing correct code for handling data generically very difficult, if absence of values needs to be handled distinctly from other potential error paths.

(Why is this an issue now, and not earlier? It's always been an issue -- but sometimes it's avoidable. The typed Node implementations have generally had clearer behavior for this. In many cases, a data transformation that's not conditional on the existing data hits no speedbumps here. But in completely generic data transforms -- such as a Patch implementation, which I'm working on now, it's becoming quite keening indeed.)

At the same time: I'm finding that I experience 'nil' considerably more often than I'm happy with while using this library. One of the biggest sources of this is when nil is used where an Absent value might also be semantically reasonable.

To address both of these problems, here is what I think we should do:

  • Node implementations should return Absent for the value when doing lookups in a map or list, if that value is indeed absent. (No longer should this ever use 'nil'.)
  • Nodes should not return datamodel.ErrNotExists anymore. Returning Absent as the value and nil as an error should be the new style. (Trying to handle value absence as an error path has turned out to be considerably painful, and often produces confusing branching.)
  • ErrNotFound: this might still exist, but it's should only used by APIs that are doing more than one stride at a time (e.g. traversal stuff).
  • While we're at it: In typed nodes, structs also should return Absent for both a field which is defined but was absent, and also for a field which is not defined. (Previously, structs returned somewhat stronger error for a field that is not defined: schema.ErrNoSuchField. But I've come to think this is probably on net unhelpful; the general learning from working with our type system over time seems be uncovering that the less the type lens makes things diverge from the basic data model, the better.)

This is likely to be a breaking change in a few cases. Worse, it's the kind that the compiler won't detect. Nonetheless, I think pulling the bandaid on this one will not become easier over time, so it might as well be done soon or now.

Previous review: #74 (no final resolution reached, but covers the same problem).
Possibly also relevant (though tangentally): #191

We might consider how to migrate this smoothly. For example, perhaps there could be an intermediate time where we return both Absent and ErrNotExists. However, it's not immediately clear that this would actually result in greater smoothness of transition. AMEND: Let's not atttempt this particular "migration" strategy. It's sufficiently un-idiomatic golang that it seems like a very poor idea, even temporarily.

Metadata

Assignees

Labels

P2Medium: Good to have, but can wait until someone steps up

Type

No type

Projects

  • Status

    🗄️ Backlog

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions