Replies: 13 comments 13 replies
-
I think this is very important, we definitely need have a standard way to return errors (here I'm not sure if we have many different patterns in place). I think the errors should be validated/propagated following this order:
So, I'd merge the items "2" and "3" from your list. |
Beta Was this translation helpful? Give feedback.
-
Difference between different inputsI think its fine to keep 2 & 3 together. It would be hard for API creator to think about whats field input and whats some other user input. Its all input :-). Nested fieldsFrom what I can we use Laravel validation, so I think that guide us how the nested fields will be presented - https://laravel.com/docs/11.x/validation#validating-nested-array-input Error typeAgree with Jonas that there is not much value of adding type. That can be covered with http code. 500For 500 errors - these are unexpected errors - so unlikely to be able to communicate accurate error to the user. Not sure if was in past was considered to expose stack trace, so users have something to share on the forum. This is often considered as bad security practice revealing internals - but with opensource software thats maybe not concern. Current errors and consistencyOf course everyone agree on consistency. But I suspect someone would need to investigate how good/bad are we with consistency with existing API. Couple of endpoints I checked for validation error returns 400 with the object of errors (Example). Which I don't have any problem at all. It has all the information I need on frontend. Shape of errors for multilingual fieldsOne thing thats specific for OJS/OMP/OPS are multilingual fields. Don't have much insights at this point, apart from that we might need to communicate validation errors multilingual as well for multilingual fields. I can explore bit more with Devika, how we might want to communicate validation errors in the future forms if that would help. Validation errors translationsThats something I need to understand more - how that has been approached so far. |
Beta Was this translation helpful? Give feedback.
-
I also prefer with HTPP error codes as types. ORCID is using 500 Error codes to return different types of errors extensively and is very helpful in testing. https://github.com/ORCID/ORCID-Source/blob/main/orcid-api-web/tutorial/api_errors.md |
Beta Was this translation helpful? Give feedback.
-
@ewhanson Hi! Just tagging you, as you might help to conclude with some guidance. |
Beta Was this translation helpful? Give feedback.
-
For the invitation API I am proposing the following validation strategy:
It leaves room for already defined validation rules and custom ones. OJS already has a Validator defined
This is still a work in progress, but I think it makes sense to establish some ground rules regarding the strategy we should follow, so any thoughts and considerations are helpful and welcome. |
Beta Was this translation helpful? Give feedback.
-
This all sounds really well thought out. Here are some of my initial thoughts: In general, I think sticking as close to Laravel conventions as possible will serve us well (which we do to a large degree already). It provides a well thought-through structure and will allow us to benefit from additions and enhancements in the future. For UI Field validation responses, I'd propose we follow the Laravel convention outlined in the XHR Request and Validation and Validation Error Response Format section of the Laravel docs. This would standardize a format of the following for responses with a situation-specific {
"message": "The team name must be a string. (and 4 more errors)",
"errors": {
"team_name": [
"The team name must be a string.",
"The team name must be at least 1 characters."
],
"authorization.role": [
"The selected authorization.role is invalid."
],
"users.0.email": [
"The users.0.email field is required."
],
"users.2.email": [
"The users.2.email must be a valid email address."
]
}
} and responding with Echoing the others, I also prefer letting the HTTP status code convey the semantics of the response rather than further details in the JSON. @jonasraoni mentioned the other response codes aside from Using this format for more generic errors that aren't tied to fields would work as well, there just wouldn't be any fields with field-specific "errors" in the {
"message": "Localized error message"
} In @defstat's example of this, there's an array of business logic validation errors. I'm curious if you had a specific use case where it makes sense to return multiple errors like this that are not tied in any way to specific fields. I'll spend a bit of time looking through the existing endpoints with this kind of approach in mind and see if any obvious issues/pain points pop up as well. |
Beta Was this translation helpful? Give feedback.
-
Submission wizard logicJust looking more what strategy the submission wizard process has taken. Thats more complex use case, as there is multiple api endpoint to interact with, to include all the data, but still useful to explore that. Whats interesting that for example title&abstract are sent to publication endpoint. And that skips the validation as it consider 'submissionProgress' . Therefore it relies on the client side to enforce the title at that moment. But it does the validation of 'everything' on last 'Review' page. Using the POST Creating invitationContextWhats @Devika008 has been asking from UX perspective is to provide more instant feedback if something is missing, not at the end of the wizard. So we want to make sure that every page is properly filled before moving user to the next. And its not just about fields being required. For example when editor is entering role and the start/end date. These dates needs to be validates not just have sensible format, but also sensible dates. So we did not really attempt to do these validations on client side, as api needs to validate these things regardless so it can't be bypassed. Therefore implementing such validation on client would be simply duplicated effort, which might slightly improve UX and make it even more instant, but providing the feedback from API when user attempts to go to the next page I think is good balance between complexity and UX (@Devika008 feel free to chip in on this). What I have been advocating for is that the 'update' endpoint for validation always returns all things that are still wrong. And because frontend knows which fields are being currently presented - it shows relevant subset of the errors, ideally collocated with actual fields. I think what @defstat described - it currently works like that, right? I remember initially you attempted to validate only fields that are included in the request - but now you are validation everything, correct? Question to exploreOnly outstanding thing from my side that I wanted to ask you about is the 'partial updates'. For invitations its probably not big deal - but it could be slight UX improvement if user gets pass certain screen (so the API endpoint is hit) - that it would save the data that are passing validation (in case he abandons the process and opens the invitation later on to finish it). Theoretically should be possible - that whatever field pass validation gets saved. But wanted to ask whether such approach creates some pain points on api implementation? Also if you think its bad idea for some reason, please share these thoughts. |
Beta Was this translation helpful? Give feedback.
-
Thanks @defstat for elaborating, thats helpful. And I do like your proposal more compared to what I was proposing. In general when we are creating new object (like new contributor) it make sense to require everything at once, but in this case where we have process of creating invitation, than refining and submitting - I think what you describe is better fit. Will try to summarise:
@ewhanson Can you just double check this from your point of view? Once you give it green light, @ipula could start working on client side adjustments. |
Beta Was this translation helpful? Give feedback.
-
Thanks @jardakotesovec and @defstat. That all sounds conceptually good to me, and @jardakotesovec your summary fits with my understanding as well. If we do go for any "partial updates," I'd like to have a look once it's minimally working to make sure we're all on the same page for implementation, but otherwise 👍 |
Beta Was this translation helpful? Give feedback.
-
@jardakotesovec @ewhanson, just a small comment here: performing a "validity" check at these terminal steps of the invitation process is not merely a technicality for the following reasons: Unvalidated Properties: There may be properties that were never sent to the populate or refine calls and therefore have not undergone any validation. These properties could be essential during the invite/finalize stages. For instance, consider the User Groups changes for an invitation that assigns a user to specific groups. The property defining which user groups are being assigned is required for the invite action. However, we cannot tag this property as required initially, because doing so would force us to provide this information during every populate action, making it the first step in the wizard. Time-sensitive Validations: Some properties are validated against rules that may yield different results over time. A good example is the "email" property in an invitation, which highlights the distinction between validation errors and other exceptions. When finalizing the invitation, the system might need to create a new user. The email associated with the "Invite" stage may have already been checked for uniqueness against the Users table. However, if the invitation is finalized several days later, the email's uniqueness might need to be rechecked. In the initial version of the backoffice code, this check returned an exception if it failed. However, I changed this to a validation error so that the UI can display a user-friendly message. |
Beta Was this translation helpful? Give feedback.
-
Hi @jardakotesovec @defstat @ewhanson I fully agree with the above discussion.#10190 (comment) if we only want to see for some alternative ideas, we can put in the payload for |
Beta Was this translation helpful? Give feedback.
-
I completely agree with the above discussion #10190 (comment), and also the limitations described by @defstat I think this aligns really well with the error validation logic I mentioned earlier. |
Beta Was this translation helpful? Give feedback.
-
In reply to @touhidurabir's comment: For the needs of the GDPR work that is currently under development, and the backoffice of Invitations specifically we have merged into main the following approach (this is an example): Declaration of validation rules: Invocation within the context of a trait That approach is trying to be consistent with Laravel Validation framework/approach and also be used for both API calls and direct usage from the backoffice code. Is that consistent with what you are exploring @touhidurabir? Do you think that we should re-organize the pattern used there? |
Beta Was this translation helpful? Give feedback.
-
Hi team,
I would like to start a discussion on how our API endpoints should handle and return validation errors. On the work on the Invitations API, there is a need to ensure that validation errors are returned in a consistent and informative manner, distinguishing between
We should consider a standardized and documented way of returning such errors which will lead to more clarity and consistency in how errors are handled on the front-end and back-end.
There is already a way to return errors (see for example this). I can't see that documented though so that it is clear for the API developers and consumers on how to process different kinds of errors.
Proposal
400 Bad Request
or422 Unprocessable Entity
422 Unprocessable Entity
500 Internal Server Error
Benefits
Further Questions
field.subfield
sufficient)Beta Was this translation helpful? Give feedback.
All reactions