IWF-808 use Unset object for empty data output
#106
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
N-give
requested review from
dlukiantsev,
ktrops,
lwolczynski,
mixydubs,
samuel27m and
steven-rg-harrison
N-give
commented
Apr 28, 2025
Comment on lines
+172
to
181
| payload: Union[EncodedObject, Unset] = Unset() | ||
| is_encoded = False | ||
| for converter in self.converters.values(): | ||
| payload = converter.to_payload(value) | ||
| if payload is not None: | ||
| is_encoded, payload = converter.to_payload(value) | ||
| if is_encoded: | ||
| break | ||
| if payload is None: | ||
| if not is_encoded: | ||
| raise RuntimeError( | ||
| f"Value of type {type(value)} has no known converter", | ||
| ) |
Contributor
Author
There was a problem hiding this comment.
Here we were overloading None to also mean not yet encoded, but that shouldn't always be the case. If no data comes in, we can return no data.
iwf/object_encoder.py
Outdated
Comment on lines
233
to
241
| def encoding(self) -> Union[str, Unset]: | ||
| """See base class.""" | ||
| return "binary/null" | ||
| return UNSET | ||
|
|
||
| def to_payload(self, value: Any) -> Optional[EncodedObject]: | ||
| def to_payload(self, value: Any) -> tuple[bool, Union[EncodedObject, Unset]]: | ||
| """See base class.""" | ||
| if value is None: | ||
| return EncodedObject( | ||
| encoding=self.encoding, | ||
| ) | ||
| return None | ||
| return (True, Unset()) | ||
| return (False, Unset()) |
Contributor
Author
There was a problem hiding this comment.
To get the response to have data=null like the java-sdk, this needed to return Unset instead of an EncodedObject with an encoding and a empty data. The to_dict method generated for the response object will include anything that isn't Unset
Comment on lines
+233
to
+235
| def encoding(self) -> Union[str, Unset]: | ||
| """See base class.""" | ||
| return "binary/null" | ||
| return UNSET |
Contributor
Author
There was a problem hiding this comment.
The CompositePayloadConverter stores all of the converters in a dict by their encoding, but when the server sends empty data, the encoding field of the EncodedObject will be UNSET, so for this to properly match, its encoding needs to also be UNSET.
samuel27m
reviewed
Apr 29, 2025
samuel27m
approved these changes
Apr 29, 2025
lwolczynski
approved these changes
Apr 29, 2025
dlukiantsev
approved these changes
Apr 29, 2025
N-give
added a commit
that referenced
this pull request
May 6, 2025
* IWF-808 use `Unset` object for empty data output --------- Co-authored-by: Nathan Givens <ngivens@indeed.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.