Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APIv2: proto: rename IsInitialized as CheckInitialized? #887

Closed
dsnet opened this issue Jul 3, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@dsnet
Copy link
Member

commented Jul 3, 2019

I find it odd that a function named IsInitialized returns an error, rather than a bool. Perhaps this should be renamed to CheckInitialized?

Alternatively, we can change IsInitialized to simply return a bool and add CheckInitialized later. It's not clear to me what the use-case is for returning an error:

  • If the purpose is only to programmatically identify a required field check error, then a boolean is fine.
  • If the purpose is to programmatically identify which required field was not set, then an error alone is unhelpful unless we provide the means to interpret it. However, there are two ways to represent the location of failure, and I've seen desires for both:
    • Identify by path. For example, .my_field.repeated_field[5].map_field["key"].required_field
    • Identify by field full name. For example, "fizz.buzz.MyMessage.NestedMessage.required_field"
    • The current implementation identifies the field by path, while it seems that some usages in want it by full name.
  • If the purpose is human debugging, then I guess an opaque error can be helpful, but I'm not sure how often this is the case.

\cc @neild

@dsnet dsnet added the api-v2 label Jul 3, 2019

@neild

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

Changing to a bool return SGTM. As a bonus, it'll be faster since getting the human-readable error currently hits the slow path.

We should also decide what to do when an initialization check fails in Marshal/Unmarshal: Currently we do a slow-path check to get a nice, human-readable error with the path of the error. I'm not sure that's the right behavior; falling off a performance cliff when you get bad data is not friendly. I'm tempted to say that we should return the full name of the field from both the fast and slow paths (which I believe should be easy without compromising performance) which is probably good enough for debugging and equivalent to what v1 does.

@dsnet dsnet added this to the APIv2 release milestone Jul 7, 2019

@dsnet dsnet added the blocks-v2 label Jul 7, 2019

@dsnet dsnet removed this from the APIv2 release milestone Jul 7, 2019

@neild

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Looked at this a bit, and a few thoughts:

  • The C++ API has a CheckInitialized function which CHECK-fails if the message isn't initialized. While we don't need to follow C++'s naming in all cases, I'd rather avoid functions with the same name but dangerously different behavior.
  • We need an exported initialization check function that returns a human-readable error for use by encoding/prototext and encoding/protojson.
  • I don't like our current behavior of returning one type of error (the field name) from the fast path and then redoing the check on the slow path to produce a different type of error (the path). We shouldn't have wildly different performance when presence checks succeed vs. fail. Also, I found one convoluted scenario where we can actually end up doing O(nesting depth) redundant slow-path checks.
  • If you want programmatic access to missing required fields, you probably want all the missing fields not just the first one.

My current thought on what to do:

  • Keep the name IsInitialized.
  • IsInitialized returns an error. It's a little bit weird, but not too bad.
  • Required-not-set errors always identify the offending field by its full name.
  • No slow-path fallback, since the fast and slow paths both return exactly the same error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.