-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[azure-search] Serialization fails due to Cannot map a recusive structure #18542
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code change looks good, some minor remarks on the test cases
sdk/search/search-documents/test/internal/serialization.spec.ts
Outdated
Show resolved
Hide resolved
{ id: "3", children: [child] } | ||
]; | ||
const result = serialize(documents); | ||
assert.deepEqual(documents, result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this fail because the result shape is {hello: 'INF', world: '-INF', universe: 'NaN'}
instead of the number value after serialization?
sdk/search/search-documents/test/internal/serialization.spec.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Jeff Fisher <xirzec@xirzec.com>
Co-authored-by: Jeff Fisher <xirzec@xirzec.com>
…ture (Azure#18542) * Continue on a recursive structure walking * Update Changelog * Update sdk/search/search-documents/test/internal/serialization.spec.ts Co-authored-by: Jeff Fisher <xirzec@xirzec.com> * Update sdk/search/search-documents/test/internal/serialization.spec.ts Co-authored-by: Jeff Fisher <xirzec@xirzec.com> * Fix Typo Co-authored-by: Jeff Fisher <xirzec@xirzec.com>
This PR is to fix the issue reported at #15656. As discussed offline, we do not want to error out. Instead we can continue. I have also added the required test cases during the serialize/deserialize step.
@xirzec Please review and approve the PR.