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

[Serializer] [DX] NotNormalizableValueException should note which attribute failed to denormalize when possible #31049

Conversation

Projects
None yet
6 participants
@Simperfit
Copy link
Contributor

Simperfit commented Apr 10, 2019

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #29230
License MIT
Doc PR

This is the idea of a fix discussed with @stof about the improvment of serializer error message. Now we can try a implementation lis this one. This is linked to EUFOSSA even if this has been done not in the hackathon.

@jewome62
Copy link
Contributor

jewome62 left a comment

I think the word Protip is not mandatory

@Simperfit Simperfit force-pushed the Simperfit:dx/improve-error-message-on-not-normalized-exception branch from 09962d4 to 668a13b Apr 10, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

Simperfit commented Apr 10, 2019

Status: Needs Review

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 10, 2019

@Simperfit Simperfit force-pushed the Simperfit:dx/improve-error-message-on-not-normalized-exception branch from 668a13b to f251321 Apr 10, 2019

@ogizanagi

This comment has been minimized.

Copy link
Member

ogizanagi commented Apr 10, 2019

I think it's a great step forward. But to me it should go even beyond this:

  • track exactly the path in the graph at which this occurs (the difficult part).
  • add it to the exception as a property, so it can be used programatically. Such exceptions are probably not supposed to produce 500 responses in an API for instance. When catching it, this would allow better generation of error responses.

Nevertheless, this first step is already great for debugging.

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

Simperfit commented Apr 10, 2019

@ogizanagi I think we could do that but in a second way, this is already nice to have.

  1. In the DateTimeNormalizer AFAIK we don't have the graph information.

  2. That's it, as we discussed with @stof, I think we should go in the object normalizer everytime, catch this and try to have the most information out of this in order to give the users all the information about what happen and what the error is, so no 500 for APIs.

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

Simperfit commented Apr 12, 2019

@ogizanagi WDYT as merging this one as is, and creating an issues to follow this.

@ogizanagi
Copy link
Member

ogizanagi left a comment

WDYT as merging this one as is, and creating an issues to follow this.

Yes. As I said, this would be a nice first step.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 15, 2019

Status: needs work

@Simperfit Simperfit force-pushed the Simperfit:dx/improve-error-message-on-not-normalized-exception branch from f251321 to 0091665 Apr 16, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

Simperfit commented Apr 16, 2019

Status: Needs Review

@Simperfit Simperfit force-pushed the Simperfit:dx/improve-error-message-on-not-normalized-exception branch from 0091665 to 6dc2b21 Apr 16, 2019

[Serializer] [DX] NotNormalizableValueException should note which att…
…ribute failed to denormalize when possible

@Simperfit Simperfit force-pushed the Simperfit:dx/improve-error-message-on-not-normalized-exception branch from 6dc2b21 to 59942b4 Apr 17, 2019

@Simperfit

This comment has been minimized.

Copy link
Contributor Author

Simperfit commented Apr 17, 2019

done @ogizanagi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.