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(dbless): drain error table when flattening errors #10256

Merged
merged 2 commits into from
Mar 28, 2023

Conversation

flrgh
Copy link
Contributor

@flrgh flrgh commented Feb 7, 2023

This only affect POST /config?flatten_errors=1.

When nested errors are flattened for the response body, they should be removed from the original error table so that they are not included in the final fields error object. Errors that are not flattened should remain.

It could be argued that this is a backwards-incompatible change, but:

  1. This is opt-in only via the flatten_errors query param.
  2. Because flatten_errors is a new, yet unreleased feature, there is no defined behavior to be broken.
  3. It's arguably a behavioral fix since we are effectively de-duplicating the errors in the response.
  4. This makes the response object much, much cleaner.
  5. The overall "shape" of the fields object has not changed.

flrgh added a commit that referenced this pull request Feb 7, 2023
@flrgh flrgh added this to the 3.2 milestone Feb 7, 2023
@flrgh
Copy link
Contributor Author

flrgh commented Feb 7, 2023

@RobSerafini This is a fix, but it's not really a blocker for anyone, so feel free to remove from the 3.2 milestone if we're too strapped for time.

Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

No concern from our side. We aren't using fields at all (with or without the availability of flattened errors--we do use messages for logging though), and I agree that if you've opted into flattened that you shouldn't need the old format also.

@RobSerafini
Copy link
Contributor

@tyler-ball - I leave it to you to decide the milestone question.

@flrgh flrgh removed this from the 3.2 milestone Feb 9, 2023
@flrgh
Copy link
Contributor Author

flrgh commented Feb 9, 2023

This will need to wait; removed from milestone.

@flrgh flrgh marked this pull request as draft February 9, 2023 01:40
@flrgh
Copy link
Contributor Author

flrgh commented Feb 9, 2023

Converting this back to a draft because I will need to make a changelog edit or two before it can be merged.

This only affect `POST /config?flatten_errors=1`.

When nested errors are flattened for the response body, they should
be removed from the original error table so that they are not
included in the final `fields` error object. Errors that are not
flattened should remain.

It could be argued that this is a backwards-incompatible change, but:

1. This is opt-in only via the `flatten_errors` query param.
2. It's arguably a behavioral fix since we are effectively
   de-duplicating the errors in the response.
3. This makes the response object much, much cleaner.
@flrgh flrgh force-pushed the fix/dbless-flattened-errors-drain branch from fd9a6b0 to 66ff8a0 Compare March 22, 2023 00:11
@flrgh flrgh marked this pull request as ready for review March 22, 2023 00:11
@flrgh flrgh force-pushed the fix/dbless-flattened-errors-drain branch from 66ff8a0 to 1763c7e Compare March 22, 2023 00:12
@flrgh
Copy link
Contributor Author

flrgh commented Mar 22, 2023

Changelog entry was added, and I've just rebased on master. This is good to go once CI is 🟢.

@hbagdi hbagdi requested a review from samugi March 23, 2023 00:19
@flrgh flrgh merged commit c550c11 into master Mar 28, 2023
@flrgh flrgh deleted the fix/dbless-flattened-errors-drain branch March 28, 2023 17:02
team-gateway-bot pushed a commit that referenced this pull request Mar 28, 2023
* fix(dbless): drain error table when flattening errors

This only affect `POST /config?flatten_errors=1`.

When nested errors are flattened for the response body, they should
be removed from the original error table so that they are not
included in the final `fields` error object. Errors that are not
flattened should remain.

It could be argued that this is a backwards-incompatible change, but:

1. This is opt-in only via the `flatten_errors` query param.
2. It's arguably a behavioral fix since we are effectively
   de-duplicating the errors in the response.
3. This makes the response object much, much cleaner.

* docs(changelog): update for #10256

(cherry picked from commit c550c11)
flrgh added a commit that referenced this pull request Mar 28, 2023
* fix(dbless): drain error table when flattening errors

This only affect `POST /config?flatten_errors=1`.

When nested errors are flattened for the response body, they should
be removed from the original error table so that they are not
included in the final `fields` error object. Errors that are not
flattened should remain.

It could be argued that this is a backwards-incompatible change, but:

1. This is opt-in only via the `flatten_errors` query param.
2. It's arguably a behavioral fix since we are effectively
   de-duplicating the errors in the response.
3. This makes the response object much, much cleaner.

* docs(changelog): update for #10256

(cherry picked from commit c550c11)

Co-authored-by: Michael Martin <[email protected]>
@hbagdi
Copy link
Member

hbagdi commented Mar 29, 2023

Does this need an explicit cherry pick from CE to EE?

@hbagdi
Copy link
Member

hbagdi commented Apr 3, 2023

@flrgh @rainest Please cherry-pick this PR to EE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants