-
Notifications
You must be signed in to change notification settings - Fork 2
Don't get confused with default message #103
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
Conversation
bhalsey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to introduce a generic invalidEnum message in fusionauth-app before we use this, right?
Yes. But if we don't, for any given code path, there should be no regression or change in behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the impact of the PMVC change on FusionAuth is that API requests using the enum for the field type will now return the [invalidEnum]field.name error. These currently return a JSON parsing exception that has the detail a dev might need, but it's not very friendly.
But if we don't, for any given code path, there should be no regression or change in behavior.
If we ship the PMVC update and miss defining the [invalidEnum] error code on some context, I think that's an exception for a missing message key. That would be a change in behavior, right?
| String message = null; | ||
| if (matchesEnumNotValidValue.matches() && matchesEnumNotValidValue.groupCount() == 2) { | ||
| code = "[invalid]%s".formatted(field); | ||
| code = "[invalidEnum]%s".formatted(field); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
invalidEnum feels a little off to me, but I can't put my finger on why. Too Java focused? Too much detail?
Is there something more generic like invalidOption or invalidSelection that would work?
If no one else has an issue, we can keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Push includes this change. Arguably not a patch release. In practice, not sure how much matters.
It was supposed to fallback to Jackson, but reading the code again, this is not tested, and not likely what would happen. I will go ensure this is tested and works. |
Tested this (and this type of mock was not doing a lot of us here, so removed that). |
If we use
[invalid], applications that define a default message will show a rather empty, generic message, rather than the Jackson message, unless a specific message is defined.This was not the desired behavior. The desired behavior was to allow an application to define a more specific message if it wanted to, otherwise fall back to the default Jackson message.