fix: Upgrade protobufjs and fix legacy key decoding in Datastore#8088
Conversation
Upgrades protobufjs to ^7.5.5 to address security vulnerabilities. Introduces a fix for legacy App Engine key decoding by explicitly calling .setup() on Reference, Path, and Element types. This ensures that group support (wire types 3 and 4) is correctly initialized in newer versions of protobufjs, which is required for these legacy keys. Fixes the "invalid wire type 4" error in unit tests. Co-authored-by: danieljbruce <8935272+danieljbruce@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Code Review
This pull request updates the protobufjs dependency to version ^7.5.5 and adds explicit initialization for the Reference, Path, and Element types in entity.ts to ensure group support for legacy keys. A review comment suggests refactoring the repetitive type lookups and setup calls into a loop to improve maintainability.
Upgrades protobufjs to ^7.5.5 to address security vulnerabilities (GHSA-xq3m-2v4x-88gg). Resolves a functional regression in decoding legacy App Engine keys by explicitly calling .setup() on Reference, Path, and Element types. This forces the generation of optimized decoders that correctly support proto2 groups, preventing "invalid wire type 4" errors. Adds 10+ regression tests for legacyDecode covering various scenarios. Co-authored-by: danieljbruce <8935272+danieljbruce@users.noreply.github.com>
Upgrades protobufjs to ^7.5.5 to address security vulnerabilities (GHSA-xq3m-2v4x-88gg). Resolves a functional regression in decoding legacy App Engine keys by explicitly calling .setup() on Reference, Path, and Element types. This forces the generation of optimized decoders that correctly support proto2 groups, preventing "invalid wire type 4" errors. Adds 10+ regression tests for legacyDecode and updates all legacy key tests to verify that legacyDecode and legacyEncode are inverses of each other. Co-authored-by: danieljbruce <8935272+danieljbruce@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…185677337093' of https://github.com/googleapis/google-cloud-node into fix-datastore-legacy-decode-protobufjs-upgrade-16655518185677337093
| // In newer versions of protobufjs, this ensures group support | ||
| // is preserved for legacy keys. | ||
| for (const typeName of ['Reference', 'Path', 'Element']) { | ||
| (loadedRoot.lookupType(typeName) as any).setup(); |
There was a problem hiding this comment.
Can we remove the as any? If possible we should just import and use the correct type.
There was a problem hiding this comment.
Yes. It looks like when we remove as any then this works just fine.
| ); | ||
| }); | ||
|
|
||
| const TEST_PROJECT = 'test-project'; |
There was a problem hiding this comment.
Should this line and below just be its own test? Nesting it's within it's seems needlessly nested for the reader.
Alternatively, you could make a new root describe to group the tests? For example:
const testCases = [...];
describe('decoding keys`, () => {
testCases.foreEach(tc => {
it(`should decode "${tc.name}"`, () => {...});
});
});
describe('encoding keys`, () => {
testCases.foreEach(tc => {
it(`should ecode "${tc.name}"`, () => {...});
});
});
There was a problem hiding this comment.
Good idea. I added a describe block to group the tests. I'm not sure what you mean by nesting it inside it though.
There was a problem hiding this comment.
Before, if I understood correctly, we were doing something like this:
it('root test' () => {
tests.forEach(t => {
// Here we have an `it`, inside another `it`.
it(`child test ${t.name}`, () => {
});
})
});
This isn't the convention I have seen in the past. It's common to nest tests in a describe, but not tests in tests.
danieljbruce
left a comment
There was a problem hiding this comment.
Added a bit of cleanup.
| // In newer versions of protobufjs, this ensures group support | ||
| // is preserved for legacy keys. | ||
| for (const typeName of ['Reference', 'Path', 'Element']) { | ||
| (loadedRoot.lookupType(typeName) as any).setup(); |
There was a problem hiding this comment.
Yes. It looks like when we remove as any then this works just fine.
| ); | ||
| }); | ||
|
|
||
| const TEST_PROJECT = 'test-project'; |
There was a problem hiding this comment.
Good idea. I added a describe block to group the tests. I'm not sure what you mean by nesting it inside it though.
…gleapis#8088) * fix: upgrade protobufjs and fix legacy key decoding Upgrades protobufjs to ^7.5.5 to address security vulnerabilities. Introduces a fix for legacy App Engine key decoding by explicitly calling .setup() on Reference, Path, and Element types. This ensures that group support (wire types 3 and 4) is correctly initialized in newer versions of protobufjs, which is required for these legacy keys. Fixes the "invalid wire type 4" error in unit tests. Co-authored-by: danieljbruce <8935272+danieljbruce@users.noreply.github.com> * fix: upgrade protobufjs and fix legacy key decoding Upgrades protobufjs to ^7.5.5 to address security vulnerabilities (GHSA-xq3m-2v4x-88gg). Resolves a functional regression in decoding legacy App Engine keys by explicitly calling .setup() on Reference, Path, and Element types. This forces the generation of optimized decoders that correctly support proto2 groups, preventing "invalid wire type 4" errors. Adds 10+ regression tests for legacyDecode covering various scenarios. Co-authored-by: danieljbruce <8935272+danieljbruce@users.noreply.github.com> * fix: upgrade protobufjs and fix legacy key decoding with inverse tests Upgrades protobufjs to ^7.5.5 to address security vulnerabilities (GHSA-xq3m-2v4x-88gg). Resolves a functional regression in decoding legacy App Engine keys by explicitly calling .setup() on Reference, Path, and Element types. This forces the generation of optimized decoders that correctly support proto2 groups, preventing "invalid wire type 4" errors. Adds 10+ regression tests for legacyDecode and updates all legacy key tests to verify that legacyDecode and legacyEncode are inverses of each other. Co-authored-by: danieljbruce <8935272+danieljbruce@users.noreply.github.com> * Apply suggestion from @gemini-code-assist[bot] Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * fix: Upgrade protobufjs and fix legacy key decoding in Datastore * Remove the as any cast * Add a describe block for the encode/decode tests --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: danieljbruce <8935272+danieljbruce@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Description
This pull request provides a better fix for what #8071 intends to fix. It resolves the protobufjs dependency issue and it ensures that unit tests don't break.
Impact
Testing
Tests have been added to ensure the legacyEncode and legacyDecode functionality remain the same. The only difference will be that with the new src changes, code that used to produce an
invalid wireerror will now produce a valid output.Generated notes
Upgrades
protobufjsto^7.5.5to address security vulnerabilities and fixes a regression in legacy App Engine key decoding. The fix involves calling.setup()on the affected protobuf types to ensure group support is preserved.PR created automatically by Jules for task 16655518185677337093 started by @danieljbruce