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

Return error message as objects instead of string #176

Closed
wesleybl opened this issue Dec 21, 2023 · 6 comments
Closed

Return error message as objects instead of string #176

wesleybl opened this issue Dec 21, 2023 · 6 comments

Comments

@wesleybl
Copy link
Member

Errors are turned into a string when they are returned, as can be seen in:

message = str(exception)

However, validation errors are lists in plone.restapi:

https://github.com/plone/plone.restapi/blob/7e445b0d6ce5158dbea4f2e727a9aa84b7a2cc43/src/plone/restapi/deserializer/dxcontent.py#L52

When these errors reach Volto, we need to treat them as an array, but they are strings. We would have to use an eval to convert, which is not very recommended. See the discussion at: plone/volto#1868

It would be nice if the errors were already seen as a list in the return json. This may break some tests but I think it's the best solution.

@davisagli
Copy link
Member

I consider it a bug that errors with a structure like this are flattened to a string rather than serialized as JSON. However, we need to make sure that if we change it, it ends up making things less broken rather than more broken in volto.

So, I think we should keep the current behavior in most cases, and implement a different serializer for specific cases like validation errors. Probably the code here should change to check if there is a JSON serialization adapter for the exception. If so, use it instead of str()

Then in plone.restapi we can add an adapter for ValidationError specifically. Of course, we also need to make sure volto is prepared to handle this structure on the client side.

@wesleybl
Copy link
Member Author

@davisagli wouldn't just testing whether the exception is ValidationError solve it? Do we really need an adapter?

@davisagli
Copy link
Member

@wesleybl That's a fair question. An adapter might be overkill here. On the other hand, it sounds like a useful feature to be able to override the serialization of any error using an adapter.

@davisagli
Copy link
Member

@wesleybl I guess it's not straightforward to use an ISerializeToJson adapter here, since that interface is defined in plone.restapi. So yeah, go with a simple isinstance check for now.

@wesleybl
Copy link
Member Author

@davisagli I realized that the error thrown by the rest api is actually a BadRequest:

https://github.com/plone/plone.restapi/blob/7e445b0d6ce5158dbea4f2e727a9aa84b7a2cc43/src/plone/restapi/deserializer/dxcontent.py#L60

Can we filter by it?

@wesleybl
Copy link
Member Author

wesleybl commented Mar 4, 2024

It was possible to fix plone/volto#1868 without this change. See: plone/volto#5808

@wesleybl wesleybl closed this as completed Mar 4, 2024
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 a pull request may close this issue.

2 participants