Skip to content
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

fix: harmonize HTTP gateway response object when encoding fails #1859

Merged
merged 2 commits into from
Aug 22, 2023

Conversation

dcroote
Copy link
Contributor

@dcroote dcroote commented Aug 18, 2023

Closes #1845.

@bbenligiray you say in #1845:

As a note, unlike Solution 1, this keeps the rawValue name, which is misleading. This is not the raw response of the API, but rather the response that is post-processed by the Airnode.

And I agree

So if Solution 2 will be implemented, this field should be renamed to something more accurate, such as postProcessedResponse/postProcessedBody/postProcessedValue.

For consistency, wouldn't we want the field renamed in the case both of encoding success and failure? I assume yes, in which case this would be a breaking change for anyone using / expecting the HTTP gateway rawValue field. Now, we could argue that the HTTP gateway is experimental and its fields shouldn't be relied upon, but having this change in v0.12.0 --> v0.12.1 (as currently slated) feels a bit much. We could also say this is v0.13 instead of v0.12.1, but there is more process (e.g. docs release) for that.

What I've done for the moment is harmonize it so that rawValue is returned upon encoding failure like success, which is consistent if slightly inaccurate in name, but happy to make the changes throughout the codebase and docs (e.g. here, here, here, etc.) if you want it renamed.

I've tested rawValue is present and contains the expected API response from coingecko by using the CI-built client docker image and a wrong _path e.g. market_data.current_price.usd234 instead of market_data.current_price.usd.

@dcroote dcroote self-assigned this Aug 18, 2023
@bbenligiray
Copy link
Member

For consistency, wouldn't we want the field renamed in the case both of encoding success and failure?

Yes

in which case this would be a breaking change for anyone using / expecting the HTTP gateway rawValue field

Yes

Now, we could argue that the HTTP gateway is experimental and its fields shouldn't be relied upon, but having this change in v0.12.0 --> v0.12.1 (as currently slated) feels a bit much.

Fair enough

We could also say this is v0.13 instead of v0.12.1, but there is more process (e.g. docs release) for that.

Indeed

What I've done for the moment is harmonize it so that rawValue is returned upon encoding failure like success, which is consistent if slightly inaccurate in name

Isn't this as breaking as the renaming of the field (even if at a smaller scope)? I don't see why we would update the schema while holding back renaming a field.

@dcroote
Copy link
Contributor Author

dcroote commented Aug 18, 2023

Isn't this as breaking as the renaming of the field (even if at a smaller scope)?

The change here (returning the response within rawValue instead of directly) only affects new v0.12 behavior and therefore likely affects very few people (data weren't returned before when encoding failed), whereas renaming rawValue breaks relative to additional versions e.g. v0.11, v0.10, etc.

@bbenligiray
Copy link
Member

Okay, I'm convinced

@dcroote
Copy link
Contributor Author

dcroote commented Aug 21, 2023

@andreogle does this work for you? If so, would you mind giving it an approval?

Copy link
Member

@andreogle andreogle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, sorry about the delay 👍🏻

@dcroote dcroote merged commit 502a5aa into master Aug 22, 2023
25 checks passed
@dcroote dcroote deleted the dcroote/issue1845 branch August 22, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Have the non-signed HTTP gateway response schema be consistent
3 participants