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(bigquery/storage/managedwriter): correct enum processing in NormalizeDescriptor #5811

Merged
merged 3 commits into from
Mar 30, 2022

Conversation

shollyman
Copy link
Contributor

@shollyman shollyman commented Mar 29, 2022

Report from #5794 demonstrates backend rejection when you have
an external enum that's re-used across multiple nested messages.

Fix is straightforward. Enum processing was incorrectly adding the enum message in the current child, rather than within the root descriptor. This PR addresses that, and augments testing.

Fixes: #5794

Report from googleapis#5794 demonstrates backend rejection when you have
an external enum that's re-used across multiple nested messages.

Unclear if this should be resolved in the backend or if we should
detect re-use and/or build a dependency cache for this kind of thing.
@shollyman shollyman added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 29, 2022
@shollyman shollyman requested a review from a team March 29, 2022 04:40
@shollyman shollyman requested a review from a team as a code owner March 29, 2022 04:40
@shollyman shollyman requested a review from steffnay March 29, 2022 04:40
@product-auto-label product-auto-label bot added size: l Pull request size is large. api: bigquery Issues related to the BigQuery API. labels Mar 29, 2022
@shollyman
Copy link
Contributor Author

$ go test -v -run TestIntegration_ProtoNormalization/group/WithExternalEnum
=== RUN   TestIntegration_ProtoNormalization
=== RUN   TestIntegration_ProtoNormalization/group
=== RUN   TestIntegration_ProtoNormalization/group/WithExternalEnum
=== PAUSE TestIntegration_ProtoNormalization/group/WithExternalEnum
=== CONT  TestIntegration_ProtoNormalization/group/WithExternalEnum
    integration_test.go:782: error in response: rpc error: code = InvalidArgument desc = Invalid proto schema: BqMessage.proto: testdata_ExternalEnumMessage.testdata_EnumMsgB.baz: "testdata_ExtEnum_E.ExtEnum" is not defined. Entity: projects/shollyman-demo-test/datasets/managedwriter_test_dataset_20220329_16945717740246_0001/tables/table_20220329_16945717803960_0001/streams/_default
--- FAIL: TestIntegration_ProtoNormalization (1.55s)
    --- FAIL: TestIntegration_ProtoNormalization/group (0.00s)
        --- FAIL: TestIntegration_ProtoNormalization/group/WithExternalEnum (0.51s)
FAIL
exit status 1
FAIL    cloud.google.com/go/bigquery/storage/managedwriter      1.624s

@shollyman shollyman removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 29, 2022
@shollyman shollyman changed the title test(bigquery/storage/managedwriter): test embedded enums fix(bigquery/storage/managedwriter): correct enum processing in NormalizeDescriptor Mar 29, 2022
@shollyman shollyman merged commit 52cf48e into googleapis:main Mar 30, 2022
@shollyman shollyman deleted the fr-nested-enum branch March 30, 2022 00:42
shollyman added a commit to shollyman/google-cloud-go that referenced this pull request Mar 30, 2022
Make the enum append more consistent, missed during the initial
fix in googleapis#5811
shollyman added a commit that referenced this pull request Mar 30, 2022
Make the enum append more consistent, missed during the initial
fix in #5811
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bigquery: adapt.NormalizeDescriptor produces Invalid DescriptorProto with nested enums
2 participants