Improverbosity by InvalidBody showing the root cause and the reason. #184
Conversation
DEVizzent
force-pushed
the
improve-invalid-body-error-verbosity
branch
from
aa61adc to
95aad7b
Compare
| $contentType, | ||
| $addr, | ||
| substr($prev->getMessage(), 0, -1), | ||
| implode('->', $prev->dataBreadCrumb()->buildChain()) |
There was a problem hiding this comment.
I'm not really sure, this will not clutter the message for really complex schemas. Can you please provide use case for these messages?
IMO since the validation is exception-based one can wrap validation in try..catch clause and do proper logging of any needed info in any needed format. And for thing like oneOf or allOf this really can be messy as a raw string
There was a problem hiding this comment.
Most of times we I use this lib for testing, validating request and responses according documentation y got InvalidBody message. So I have to debug when this exception is thrown to see the previous one and come across the root cause of the error. I used a try catch on my side to add more context to this error, but I think it can be usefull for all.
Body does not match schema for content-type "application/json" for Response [put /post/1]
Don't provide any context about why it failed. With this change you will see:
Body does not match schema for content-type "application/json" for Response [put /post/1]. [Value expected to be 'integer', 'string' given in User -> id]
Whith this message we can identify the root cause "User -> id" and the reason.
There was a problem hiding this comment.
I've got it. Testing doesn't have that much tooling indeed. What do you think about introducing some kind of global toggle like Validator/Debug::enable() (or maybe Verbose::enable() ) in order to make it configurable ? I really want to keep generic flow simple since these messages are not machine readable in any case and format (as I specified for oneOf and allOf) can be very debatable
There was a problem hiding this comment.
I understand add a Verbose::enable() which it's only used in one exception it's not a great option. Also as you said this info can be gotten by who need it with a try catch.
So, if you like this Verbose option, and you think it will be used in the future for other purposes I will add it. If not we can close the MR.
I wait for your comment and thank you so much for your fast answers and review :)
There was a problem hiding this comment.
Other solution I can suggest is to add getVerboseMessage on this exception class, so one can just call it instead of getMessage in order to see some additional info as a plain text.
How other libs deal this? LIke symfony validator and others ?
There was a problem hiding this comment.
I really want to keep generic flow simple since these messages are not machine readable in any case and format (as I specified for oneOf and allOf) can be very debatable
Why do you want keep it readable for machines? IMO Machines can have enough with the exception type/name, but developers need a message.
There was a problem hiding this comment.
since these messages are not machine readable
The point is not keep (or make) these machine readable. I'm saying opposite here, these messages (currently and in the future) are not machine readable and not ment to be. These messages are visible in some kinds of logging solutions and, if you take for example symfony debug exception page or sentry as an exception handler - you already can get all the needed info there having chained exceptions on hand.
And adding more info to the messages will pollute simple logging solutions like ELK staks, file logs and so other.
So if we talking about tests here only, it's something about debugging and debugging is usually works in special ways (debug mode for the application with extra wrappers, enabling some debug routines, etc).
That's in my mind here. Not saying this PR is useless or bad, just want to be sure we are not doing worse in other places making one places prettier
There was a problem hiding this comment.
Sorry @scaytrase, I missunderstand you. Now I understand completly your point :)
I also see, that my last message can seem so rude, it wasn't my intention.
I know you didn't said this PR is useless or bad. But I feel that maybe the case I though it isn't common and can be handled getting the exception information. I think maybe have no sense add this getVerboseMessage only to this exception.
But, if you feel it can provide value I'll be happy to do the changes and contribute.
There was a problem hiding this comment.
I also see, that my last message can seem so rude, it wasn't my intention.
Ah, it's ok, i'm not native english speaker, so I expect some miscommunications )
I think maybe have no sense add this getVerboseMessage only to this exception.
Up to you. I this solves yours case - I think in this format it would be great addition. I saw people who might get use of it before
There was a problem hiding this comment.
Done I placed the method in the parent class. Sorry for the delay, and thank you for the support 💪
DEVizzent
force-pushed
the
improve-invalid-body-error-verbosity
branch
from
95aad7b to
7aeffca
Compare
|
Nice. I will do it :)
El sáb., 28 ene. 2023 9:21, Pavel Batanov ***@***.***>
escribió:
… ***@***.**** commented on this pull request.
------------------------------
In src/PSR7/Exception/Validation/InvalidBody.php
<#184 (comment)>
:
> @@ -17,7 +19,13 @@ public static function becauseBodyDoesNotMatchSchema(
SchemaMismatch $prev
): self {
$exception = static::fromAddrAndPrev($addr, $prev);
- $exception->message = sprintf('Body does not match schema for content-type "%s" for %s', $contentType, $addr);
+ $exception->message = sprintf(
+ 'Body does not match schema for content-type "%s" for %s. [%s in %s]',
+ $contentType,
+ $addr,
+ substr($prev->getMessage(), 0, -1),
+ implode('->', $prev->dataBreadCrumb()->buildChain())
I've got it. Testing doesn't have that much tooling indeed. What do you
think about introducing some kind of global toggle like
Validator/Debug::enable() (or maybe Verbose::enable() ) in order to make
it configurable ? I really want to keep generic flow simple since these
messages are not machine readable in any case and format (as I specified
for oneOf and allOf) can be very debatable
—
Reply to this email directly, view it on GitHub
<#184 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJW2WAUJCHXHDRUAYEQ2FITWUTJHPANCNFSM6AAAAAAUH7XCTA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***
com>
|
DEVizzent
force-pushed
the
improve-invalid-body-error-verbosity
branch
from
7aeffca to
0f8ed5e
Compare
|
Can you please use something meaningful in commit author? I'd like to preserve original authorship of commits in general, but have to squash and fix commits of previous PR because they have no name neither linked to github account via email of commit being linked to any GH account |
Sure, I'll do it tomorrow morning. I will check the commit history to find some examples |
DEVizzent
force-pushed
the
improve-invalid-body-error-verbosity
branch
from
0f8ed5e to
93ac1f3
Compare
DEVizzent
force-pushed
the
improve-invalid-body-error-verbosity
branch
from
93ac1f3 to
464eb0a
Compare
@scaytrase Yesterday I didn't understand what you were saying, because I have properly configured my global user.name and user.email, but I don't know why I wasn't configured in this concret project 🤯 |
|
@scaytrase Can you confirm commit autor is now correct and merge? Thank you 👍 . |
|
Looks good! Thanks. I'll merge it a bit later, sorry, a bit busy last days |
|
thanks @DEVizzent ! |
Most of times we I use this lib for testing, validating request and responses according documentation y got InvalidBody message. So I have to debug when this exception is thrown to see the previous one and come across the root cause of the error. I used a try catch on my side to add more context to this error, but I think it can be usefull for all.
Don't provide any context about why it failed. With this change you will see:
Whith this message we can identify the root cause "User -> id" and the reason.