feat(smithy-client): support strict union parsing#2746
feat(smithy-client): support strict union parsing#2746JordonPhillips merged 1 commit intoaws:mainfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2746 +/- ##
=======================================
Coverage ? 61.41%
=======================================
Files ? 539
Lines ? 27525
Branches ? 6722
=======================================
Hits ? 16905
Misses ? 10620
Partials ? 0 Continue to review full report at Codecov.
|
| const setKeys = new Set<string>(); | ||
| for (const [key, value] of Object.entries(asObject)) { | ||
| if (value !== null && value !== undefined) { | ||
| setKeys.add(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
Is the Set really needed? Since we are validating the object has exactly 1 key, and JS object doesn't allow multiple entries under same key, why not just Object.keys(asObject)?.length !== 1?
There was a problem hiding this comment.
You can have multiple keys where only one is not null - { a: 1, b: null, c: null } is a valid union (and a check of the # of keys would fail in this case, I think)
Set isn't being used here as a way to deduplicate. I just used it because order doesn't matter and object keys are expected to be unique anyway.
There was a problem hiding this comment.
Since Object.entries already returns a collection, it's looks unnecessary to create a Set explicitly. Will this do?
const isDefined = (value: unknow): boolean => value !== null && value !== undefined;
if (Object.values(asObject).filter(isDefined).length !== 1) {
throw new TypeError(`Unions must have exactly one non-null member`);
}
return asObject;There was a problem hiding this comment.
I'd like to include which keys were non-null in the error message when more than one key is set, which I can't do if I just filter the values.
There was a problem hiding this comment.
The suggested way is more idiomatic. But we don't feel strong about it.
AllanZhengYP
left a comment
AllanZhengYP
left a comment
There was a problem hiding this comment.
The use of Set seems unnecessary.
Unions must be JSON objects that only have one key set.
adamthom-amzn
force-pushed
the
strict-union
branch
from
93f6c03 to
be99b31
Compare
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread. |
4 participants
Description
Unions must be JSON objects that only have one key set.
Testing
Unit tests, and protocol tests against regenerated clients.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.