Validate the unknown object is either plain object of primitive #2572
Conversation
…ibrary-js into lakhveer/validateRecord
…ibrary-js into lakhveer/validateRecord
| @@ -504,3 +504,46 @@ export function validateUuid(id: string | undefined | null): void { | |||
| throw new Error('id must be a valid UUID'); | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * @beta | |||
There was a problem hiding this comment.
Probably no need for a @beta tag here since this isn't exported outside of the library
| } | ||
|
|
||
| // Check all properties of the object recursively | ||
| return Object.keys(value).every((key) => isPrimitiveOrPlainObject(value[key])); |
There was a problem hiding this comment.
I couldn't tell by looking at your unit tests: do you have any unit tests that hit line 548 and actually recurse through? That would be I guess an object with no functions at the top level but a function inside of one of its members.
There was a problem hiding this comment.
I added the second expect in this test to explicitly test the function in nested object.
it('should return false for nested structures with non-plain objects', () => {
expect(isPrimitiveOrPlainObject({ a: [1, 2, new Date()] })).toBe(false);
expect(isPrimitiveOrPlainObject({ a: { b: [1, 2, function () {}] } })).toBe(false);
});
…ibrary-js into lakhveer/validateRecord
…ibrary-js into lakhveer/validateRecord
…ibrary-js into lakhveer/validateRecord
| * | ||
| */ | ||
| export function isPrimitiveOrPlainObject(value: unknown, depth: number = 0): boolean { | ||
| if (depth > 1000) { |
There was a problem hiding this comment.
I added this change as well since you last reviewed @TrevorJoelHarris
There was a problem hiding this comment.
just curious, where the 1000 from? why we decide to use 1000 instead of other number?
There was a problem hiding this comment.
Because Anand from 3pcxp said, he does not see it going any further than that, and he recommended capping it
There was a problem hiding this comment.
nit: It would be great if you could add this context as a comment for this function to help the next developer understand the logic.
There was a problem hiding this comment.
I added a comment saying Recursion is limited to a maximum depth of 1000 to prevent excessive nesting and potential stack overflow. Does that work?
| } | ||
|
|
||
| // Check if the value is a primitive type or null | ||
| if ( |
There was a problem hiding this comment.
nit: what about this approach ( it covers all primitive types without needing to list each one individually) :
return value === null || (typeof value !== 'object' && typeof value !== 'function');
}
There was a problem hiding this comment.
The function needs to allow the nested primitives in an object as well. This check will incorrectly return false if the value was a plain object or an array of primitives.
| * Limited to Microsoft-internal use | ||
| */ | ||
| function validateQueryMessageExtensionRequest(originalRequestInfo: IQueryMessageExtensionRequest): void { | ||
| if (originalRequestInfo.commandId.length > 64) { |
There was a problem hiding this comment.
I might not know much of the background context, and I feel confused and curious about where these magic numbers (like 64, 5, 512) come from.
There was a problem hiding this comment.
They all came from 3pcxp team. In this PR, I am just dividing the already existing function into smaller functions. These numbers were added when the support was added for MEs in last December
For more information about how to contribute to this repo, visit this page.
Description
In
externalAppAuthneticationandexternalAppAutneticationForCEA, some APIs acceptIActionExecuteInvokeRequestas input. This interface has adataproperty that can be of typestring | Record<string, unknown>. The unknown type in Record is unknown. Currently, we post the value as it is to host sdk. However, it could have contained values that are not serializable.The copilot team that consumes the API confirms that the type would be either primitive or plain object. This PR validate the data and throws if that is not the case.
Main changes in the PR:
isPrimitiveOrPlainObjectinutils.tsfile.IActionExecuteInvokeRequest.dataelement inExternalAppAuthenticationandExternalAppAuthenticationForCEAcapabilities.Validation
Validation performed:
Unit Tests added:
<Yes/No>
End-to-end tests added:
<Yes/No>
Additional Requirements
Change file added:
<Yes/No>
Related PRs:
Next/remaining steps:
Screenshots: