-
Notifications
You must be signed in to change notification settings - Fork 679
fix(projectify): optimize replaceProjectIdToken performance #8358
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,9 @@ import {Stream} from 'stream'; | |||||||||||||||||||||||||||||||||||||||||||||||||
| // See the License for the specific language governing permissions and | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // limitations under the License. | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| const PROJECT_ID_TOKEN = '{{projectId}}'; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const PROJECT_ID_TOKEN_REGEX = /{{projectId}}/g; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Populate the `{{projectId}}` placeholder. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -25,33 +28,43 @@ import {Stream} from 'stream'; | |||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||||||||||||||||||||||||||||||||||||||||||||||||||
| export function replaceProjectIdToken(value: any, projectId: string): any { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (Array.isArray(value)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| value = (value as string[]).map(v => replaceProjectIdToken(v, projectId)); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (typeof value === 'string') { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (value.includes(PROJECT_ID_TOKEN)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!projectId || projectId === PROJECT_ID_TOKEN) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new MissingProjectIdError(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return value.replace(PROJECT_ID_TOKEN_REGEX, projectId); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return value; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| if (value === null || typeof value !== 'object') { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return value; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| value !== null && | ||||||||||||||||||||||||||||||||||||||||||||||||||
| typeof value === 'object' && | ||||||||||||||||||||||||||||||||||||||||||||||||||
| !(value instanceof Buffer) && | ||||||||||||||||||||||||||||||||||||||||||||||||||
| !(value instanceof Stream) && | ||||||||||||||||||||||||||||||||||||||||||||||||||
| typeof value.hasOwnProperty === 'function' | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const opt in value) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // eslint-disable-next-line no-prototype-builtins | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (value.hasOwnProperty(opt)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| value[opt] = replaceProjectIdToken(value[opt], projectId); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (Array.isArray(value)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| for (let i = 0; i < value.length; i++) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const original = value[i]; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const processed = replaceProjectIdToken(original, projectId); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (processed !== original) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| value[i] = processed; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return value; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+45
to
54
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mutating the input array in-place is a breaking change compared to the original implementation, which used To preserve the performance benefits of avoiding allocations when no placeholders are present, while maintaining safety and backward compatibility, we can use a Copy-on-Write approach. We only clone the array if we actually detect a modified element.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The original implementation was already mutating values in-place for all nested objects To address the concern about frozen arrays/objects without triggering new allocations, we implemented a Selective-Write strategy. |
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| typeof value === 'string' && | ||||||||||||||||||||||||||||||||||||||||||||||||||
| (value as string).indexOf('{{projectId}}') > -1 | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!projectId || projectId === '{{projectId}}') { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new MissingProjectIdError(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (value instanceof Buffer || value instanceof Stream) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return value; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we combine this check for Buffer and Stream with the other checks above for null and non-object primitives? |
||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| for (const key in value) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (Object.prototype.hasOwnProperty.call(value, key)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const original = value[key]; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const processed = replaceProjectIdToken(original, projectId); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (processed !== original) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this could be simplified using more modern syntax: for (const key of Object.keys(value)) {
value[key] = replaceProjectIdToken(value[key], projectId);
}I don't think we need the additional |
||||||||||||||||||||||||||||||||||||||||||||||||||
| value[key] = processed; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| value = (value as string).replace(/{{projectId}}/g, projectId); | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
surbhigarg92 marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| return value; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,7 @@ describe('projectId placeholder', () => { | |
| ], | ||
| }, | ||
| ], | ||
| simpleArray: ['A {{projectId}} Z'], | ||
| }, | ||
| PROJECT_ID, | ||
| ), | ||
|
|
@@ -74,6 +75,7 @@ describe('projectId placeholder', () => { | |
| ], | ||
| }, | ||
| ], | ||
| simpleArray: ['A ' + PROJECT_ID + ' Z'], | ||
| }, | ||
| ); | ||
| }); | ||
|
|
@@ -116,6 +118,36 @@ describe('projectId placeholder', () => { | |
| ); | ||
| }); | ||
|
|
||
| it('should return values without placeholder as-is', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a lot of assertions in both of these added test cases. Could you break this into multiple more focused tests? For example: |
||
| assert.strictEqual( | ||
| replaceProjectIdToken('no-placeholder', PROJECT_ID), | ||
| 'no-placeholder', | ||
| ); | ||
| assert.strictEqual(replaceProjectIdToken(123, PROJECT_ID), 123); | ||
| assert.strictEqual(replaceProjectIdToken(true, PROJECT_ID), true); | ||
| assert.strictEqual(replaceProjectIdToken(null, PROJECT_ID), null); | ||
| assert.strictEqual(replaceProjectIdToken(undefined, PROJECT_ID), undefined); | ||
|
|
||
| const array = [1, 2, 3]; | ||
| assert.strictEqual(replaceProjectIdToken(array, PROJECT_ID), array); | ||
|
|
||
| const object = {a: 1, b: 2}; | ||
| assert.strictEqual(replaceProjectIdToken(object, PROJECT_ID), object); | ||
| }); | ||
|
|
||
| it('should handle frozen arrays and objects without placeholders correctly without throwing', () => { | ||
| const frozenArray = Object.freeze(['no-placeholder', 123, true]); | ||
| const replacedArray = replaceProjectIdToken(frozenArray, PROJECT_ID); | ||
| assert.strictEqual(frozenArray, replacedArray); | ||
|
|
||
| const frozenObject = Object.freeze({ | ||
| prop: 'no-placeholder', | ||
| other: 123, | ||
| }); | ||
| const replacedObject = replaceProjectIdToken(frozenObject, PROJECT_ID); | ||
| assert.strictEqual(frozenObject, replacedObject); | ||
| }); | ||
|
|
||
| it('should not inject projectId into stream', () => { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const transform = new stream.Transform() as any; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.