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

add failing test for submsg type mismatch #78

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

petermueller
Copy link

I opened this PR after we noticed that some messages containing Any and OneOf messages weren't being type checked if given incorrect values. After some poking, it seemed to be any nested messages.

It also has the nasty issue that when it's decoded it looks like it populates optionals with their defaults recursively. I didn't show that in this PR as we saw it in proto3, but if you are unsure what I mean I can put together and comment with a sanitized example.

Ultimately, we were somewhat able to protect against this by calling an encode & decode on the message, and throwing an error if they aren't equal, but this was less than ideal, and requires communicating that caveat to any folks sending us messages as well.

Let me know what you think. We're slowly digging into the source for gpb since this seems like an issue with that, but I figured I'd start here.

If you'd prefer this as an issue I can move it. I was just looking through the code anyway and figured I'd write a test.

@bitwalker
Copy link
Owner

You mentioned that this is coming from gpb, did you open an issue there for this, if so, could you link to it here? In your reading of the code, did you see anything which we could potentially do to address this in exprotobuf directly?

@petermueller
Copy link
Author

I have not opened a PR with them, since from their docs it sounds like defaulting is a feature. In our original context we were doing something a little bit different than the typical usage, which we were able to work around by encoding and then decoding, and returning an :error if it changed. This doesn't help with accepting messages from others, and validating them though. We may dig into all this in a little bit. I'm not sure yet what could be done within exprotobuf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants