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

do not allow to extend same field twice to prevent the error #1784

Merged
merged 4 commits into from Feb 7, 2023

Conversation

ThePlenkov
Copy link
Contributor

@ThePlenkov ThePlenkov commented Aug 2, 2022

Fixing the issue #1783

In the case if we have a call like this:

Protobuf.Root.fromDescriptor(decodedDescriptorSet);

where decodedDescriptorSet constains files which have same field ( extension + original file ) like in my case validation.proto + google_protobuf.proto ( see the example from the referenced issue ).

@alexander-fenster
Copy link
Contributor

@ThePlenkov Would it be possible to add a unit test for this case? Thank you!

src/root.js Show resolved Hide resolved
@ThePlenkov
Copy link
Contributor Author

To test the case you need to but a breakpoint here:

return true;

Then during comp_import_extend.js test you may see that when we load proto and generate the descriptor - it works fine. But when we load one more root from the descriptor - the issue appears.

Probably the root of the problem is deeper - and the descriptor set should be generated differently - but I have no expertise to comment on that.

@ThePlenkov
Copy link
Contributor Author

Actually I realized what happens - and is not even related to import. It's only about using fromDescriptor with extended fields. I've used a standard test.proto file for that

@ThePlenkov
Copy link
Contributor Author

@alexander-fenster hi! Did you have a change to look at unit tests for this problem may be? Thank you!

@ThePlenkov
Copy link
Contributor Author

Hi! May I ask please what's the reason of this change not going through? Thanks!

@jackkav
Copy link

jackkav commented Dec 15, 2022

Thanks for the fix @ThePlenkov 🙇

@jackkav
Copy link

jackkav commented Jan 19, 2023

@alexander-fenster is there anything else blocking this PR review? It is responsible for fixing server reflection.

@alexander-fenster alexander-fenster merged commit 14f0536 into protobufjs:master Feb 7, 2023
@github-actions github-actions bot mentioned this pull request Feb 7, 2023
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.

None yet

3 participants