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

fix(spec): format validation errors for nested parameters #9775

Merged
merged 4 commits into from
Apr 10, 2024
Merged

Conversation

glowcloud
Copy link
Contributor

Refs #9774

The array parameters should now get index of the value where the error is instead of the undefined propKey. We should also go recursively into the nested errors to get a proper error message.

With these changes, the validation errors for this example input:

Screenshot 2024-04-03 at 15 28 08

should now show up as:

Screenshot 2024-04-03 at 15 28 23

We might want to try formatting them differently, ex. a.b.c: <error_message> or a[b][c]: <error_message>

I was also wondering if (perhaps in a separate issue/PR) we should show the names of the parameters. These are the values we get from

let paramValues = state.getIn(["meta", "paths", ...pathMethod, "parameters"], fromJS([]))
for the above example

{
  "query.secondParam.hash-440249344": {
    "value": "{\n  \"a\": {\n    \"a\": {\n      \"a\": \"string\",\n      \"b\": \"test\"\n    },\n    \"b\": 0\n  },\n  \"b\": 0\n}",
    "errors": [
      {
        "propKey": "a",
        "error": {
          "propKey": "a",
          "error": {
            "propKey": "b",
            "error": "Value must be a number"
          }
        }
      }
    ]
  },
  "query.param.hash-609835841": {
    "value": [
      "{\n  \"a\": \"string\",\n  \"b\": \n}"
    ],
    "errors": [
      {
        "index": 0,
        "error": "Parameter string value must be valid JSON"
      }
    ]
  }
}

If we got the names of the parameters from the keys, we could format the message as param: a: b: c: <error_message> so the actual place of the error would be visible in case if we have more than one parameter for the request.

@char0n
Copy link
Member

char0n commented Apr 9, 2024

As discussed we need to identify parameter name and use known convention for the path (lodash).

So given I have this error message returned from validation engine: Parameter string value must be valid JSON, here are the options we have to annotate the error message with parameter name and path:

Suffix form:

  • "Parameter string value must be valid JSON - location: 'paramName', path: 'a[0].b.c'."
  • "Parameter string value must be valid JSON. See 'paramName'['a[0].b.c'] for details."
  • "Parameter string value must be valid JSON, as specified for 'paramName' at 'a[0].b.c'."

Prefix form:

  • "Validation Error in 'paramName' ('a[0].b.c'): Parameter string value must be valid JSON."
  • "'paramName' > 'a[0].b.c': Parameter string value must be valid JSON."
  • "For 'paramName' at path 'a[0].b.c': parameter string value must be valid JSON."

@glowcloud
Copy link
Contributor Author

With the new changes, the parameters should now be formatted as:

Screenshot 2024-04-10 at 09 05 16

@char0n
Copy link
Member

char0n commented Apr 10, 2024

With the new changes, the parameters should now be formatted as:

Screenshot 2024-04-10 at 09 05 16

New messages look good!

@@ -494,12 +494,38 @@ export const validationErrors = (state, pathMethod) => {
let paramValues = state.getIn(["meta", "paths", ...pathMethod, "parameters"], fromJS([]))
const result = []

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about doing the following?

if (paramValues.length === 0) return result

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this to make it more clear what's happening if we don't have these values?
If so, makes sense to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this to make it more clear what's happening if we don't have these values?

Well primary factor is performance here. This is not a memoized selector, which means it's executing always fully. We want to return as soon as we can to avoid logic of creating inner functions and performing the reductions as soon as possible.

@@ -494,12 +494,38 @@ export const validationErrors = (state, pathMethod) => {
let paramValues = state.getIn(["meta", "paths", ...pathMethod, "parameters"], fromJS([]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let paramValues = state.getIn(["meta", "paths", ...pathMethod, "parameters"], fromJS([]))
const paramValues = state.getIn(["meta", "paths", ...pathMethod, "parameters"], fromJS([]))

Copy link
Member

@char0n char0n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nicely done. The logic of error extraction and formatting is complex but it's concentrated in single place which is great. I left two comments to consider before merging.

@glowcloud glowcloud merged commit 0f395c2 into master Apr 10, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants