Fix support for annotations.

[robshakir]

Through #383 we introduced a case where marshalling and unmarshalling annotations was not handled cleanly. We further had a bug whereby unmarshal was not correctly implemented such that we would continue to throw an error even whilst ignoring annotation fields.

This PR makes a few changes, listed below:

 * (M) ygot/*
 * (M) ytypes/*
   - Revert 12e41ab since this made
     a change that resulted in objects that were not necessarily JSON
     marshallable being included in output JSON.
   - Add test cases to cover rendering to multiple JSON paths.
   - Add test cases to cover unsupported unmarshalling (we're still not
     supporting this, but we were not cleanly ignoring this today.)
 * (A) integration_tests/annotations/
   - Integration check to show proto2 and proto3 being used as
     annotations in a way compatible with ygot's approach.

[robshakir]

There's something strange happening with the build environment for the GitHub actions not finding the local modules. Working to figure out what.

[wenovus]

I'm jumping to an interpretation here, so please correct if this is wrong:

I'm hoping to have a comment here clarifying that the built-in json.Unmarshal use is intentionally unmatched with the custom MarshalJSON above, and that the reason for it is so that the ultimate JSON output is more readable. Also, that when we decide to implement Annotation unmarshalling, we might have to do something different in order to preserve the custom []byte representation.

This unmatched call pair is what made an error here somewhat confusing to me, as it essentially enforces compatibility (i.e. doesn't error) between json.Unmarshal and the custom MarshalJSON while I thought that perhaps it's not guaranteed. I'm actually still not sure whether compatibility is guaranteed. If it is, can we add a comment warning the user that the custom MarshalJSON function is outputting invalid JSON, and if it is not, say that we only support it being compatible with json.Unmarshal for readability purposes?

[robshakir]

It's not really to do with readability, but rather to do with compatibility:

• MarshalJSON on the interface must return valid JSON -- and all valid JSON can be unmarshalled into interface{}. So the check here is saying "did the MarshalJSON that we just called return us something that was actually valid JSON?"
• The reason we have MarshalJSON is because not all types are by default JSON marshallable -- some of them need custom methods to be able to overload the default json.Marshal to produce valid JSON (some of them can use the JSON struct tags). For examples, see yang.Entry as something that can use struct tags to be made compatible, and any proto.Message as something that needs custom marshalling code.
• Remember here that we're not unmarshalling into the type that the user has -- e.g., if they provide us a proto message Foo that needs protojson.Marshal then we don't check that we can unmarshal and return them a new Foo, we just check that we can unmarshal into a generic container -- json.Unmarshal's success depends both on the input []byte as well as the supplied type to unmarshal into.

Unmarshalling to the user's type is more complex than this normal json.Unmarshal because we do want to give the user back the one of the types that they gave us -- in this case, there might be some custom mapping that is needed to produce that struct and not just a generic map[string]interface{} or interface{}. In that case, the user might need to give us e.g., protojson.Umarshal rather than just being able to use the default json.Unmarshal into their type.

I think the incorrect assumption in your logic is that there is valid JSON that is incompatible with json.Unmarshal(thatJSON, interface{}) - this case can't happen.

[wenovus]

Thanks for the explanation! I'll ask earlier next time instead of making assumptions to avoid this churn.

[robshakir]

No problem at all -- I missed the change that we made wasn't assuming this in the previous review :-)

Going forward, I find the best thing to do for these externally reported bugs is to write an integration test that emulates what the caller is doing - and then we get this as both a means to reproduce that issue of our own accord, as well as a future check for whether the overall workflow works. I think this gives us a bit clearer view as to where the error is too.

[wenovus]

nit: extra "!" in comment

[robshakir]

Fixed - amongst the many github-actions CI issues below :-(

[robshakir]

ping @wenovus -- PTAL. I'm going to move fixing coverage to a different PR, since there seems to be some brokenness in the actions being used here.

[robshakir]

Thanks!