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

Fix reflection access when using old generated code #6824

Merged
merged 3 commits into from
Nov 4, 2019

Conversation

jskeet
Copy link
Contributor

@jskeet jskeet commented Oct 30, 2019

Fixes #6822.

(The implementation to fix this is in the following commit.)
FileDescriptor construction uses an extension registry including extensions from imports. If these were created using an older version of protoc, the FieldDescriptor.Extension property may be null; we ignore such extensions rather than failing.
Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@jtattermusch jtattermusch merged commit 9417a31 into protocolbuffers:master Nov 4, 2019
@jtattermusch
Copy link
Contributor

This needs to be backported to 3.10.x as well?

@jtattermusch
Copy link
Contributor

Ooops it looks like I forgot to trigger the tests (The settings of the protobuf github projects don't enforce that the tests are triggered, which makes it easy to miss that)

@rafi-kamal you should add a "Required" test status for one of the test suites (e.g. "Bazel"), that way if tests are not triggered, PR will be unmergeable.
https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks

@jtattermusch
Copy link
Contributor

@rafi-kamal can you check the status of the tests on master and revert if this PR broke anything?

@jskeet
Copy link
Contributor Author

jskeet commented Nov 4, 2019

@jtattermusch: Yes, I suspect this needs to be backported to 3.10.x. Will try to create a PR for that (and the other one) in the next day or two.

@jtattermusch
Copy link
Contributor

Problem: (OldExtensions2.cs and OldExtensions1.cs) didn't get added to Makefile.am (and I forgot to trigger the tests and merged to soon because I didn't notice that).

@jskeet
Copy link
Contributor Author

jskeet commented Nov 4, 2019

I did add them to Makefile.am - should I have added them in multiple places?

@ObsidianMinor
Copy link
Contributor

The proto files were added but not the corresponding generated code.

@jskeet
Copy link
Contributor Author

jskeet commented Nov 4, 2019

Ah, whoops - will create a PR for that later. Doh.

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

Successfully merging this pull request may close these issues.

Current head C# code throws ArgumentNullException when retrieving descriptors with extensions
4 participants