Integrating Validation API On Detector Creation Process

[amitgalitz]

Signed-off-by: Amit Galitzky amgalitz@amazon.com

Description

This PR integrates the new Validation API (backend PR Link) that handles validation checks for detector settings. Currently the Validation API only includes “blocker/detector setting” level validation so the new API call will run “blocker” level validation on the detector settings as the user enters the last step of detector creation. The user will then be able to know if any of their detector settings will lead to issues that could cause detector creation to be blocked or to fail on initialization.

Changes include:

• Calling the Validation API with ‘detector’ type parameter as the “Review and Create” gets rendered.
• Validation API response will display call outs in both the detector settings and model configurations section of the “Review and Create” page. Call out will either indicate that the relative sections have been validated or display a message indicating what the error is.

Screenshots:

Validation API Response is empty, meanings all validation checks have passed:

Issue found in model configurations:

Parsing issue found in data filter query:

Need to add testing still.

Check List

• New functionality includes testing.
  • All tests pass
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[ohltyler]

How would props.validDetectorSettings be false here, but it's under the if block where !props.validationError is true?

[amitgalitz]

I guess naming is confusing but validationError is regarding any exception from validation API such as security exception is caught which should lead to no validation callout at all. So when !props.validationError is true it means props.validationError is false cause no exception was caught. props.validDetectorSettings refers to the actual ValidationAPI response and if detector settings are valid or not (data filter parsing issue and etc.) So basically if there is no exception from validate API call then validDetectorSettings can still be true or false depending on validation API response.

[ohltyler]

validationError is regarding any exception from validation API such as security exception is caught which should lead to no validation callout at all.

Is there a specific reason why not? Could be helpful to see any error here, since I'm assuming it would instantly fail when the user tries to click 'Create detector' anyways, if it is some security issue or something else.

I'm ok to leave as-is given the time, but can you add comments explaining the different names and what they mean? And/or rename the vars?

[amitgalitz]

My main logic behind this was to only use the callouts for the purpose of what the validation API is meant for. For validating against the list of checks that are blocker issues. I didn't want there to be a weird callout that I wasn't prepared for incase there are other types of exceptions that occur that I didn't think of. However I think it also totally makes sense to display security issues or other exceptions as part of validation call out but as of now I left it as the same UI as other exceptions across AD plugin. It will show up as notification on bottom right like other exceptions show. I'll add comments to explain names better and revisit this for 1.3.

[ohltyler]

Not sure if done addressing comments, but can you add some comments like you've mentioned here? Or change the names?

[ohltyler]

nit: can you make these verbose types? e.g., useState<boolean>(false)?

[ohltyler]

Another stylistic choice - I prefer to keep as few state variables as possible to prevent large amounts of vars to keep track of, especially to reduce the amount of vars needed to pass as props to child components. Can some of these be reduced, and then derived later when needed? Seems that most of these values could just be derived from the validation response.

[amitgalitz]

Every single one of those variables could be potentially passed as props to child components. I could create some of them in validation response but I am not completely sure I understand what is the better style then on the frontend.

[ohltyler]

Understood they're all needed. I was suggesting passing fewer props to the child components, by passing the full response for example, and then in the child component, pulling out whatever info is needed (like settingsResponse, featureResponse, validDetectorSettings, validModelConfigurations, validationError, etc.).

This way there's much less states needed to keep track in here, and it simplifies the logic

[ohltyler]

I'm ok to adjust now or later - if later can you open an issue to track simplifying this in the future?

[ylwu-amzn]

Can't match version in package.json: 2.1.1

[amitgalitz]

ran yarn osd bootstrap again and yarn.lock is updated

[amitgalitz]

after updating it looks like this:
first line changed to 2.1.1, not sure why line below still says version "2.4.1"

prettier@^2.1.1:
  version "2.4.1"
  resolved "https://registry.yarnpkg.com/prettier/-/prettier-2.4.1.tgz#671e11c89c14a4cfc876ce564106c4a6726c9f5c"....

[ohltyler]

^ usually if yarn finds an existing dependency in yarn.lock and it is later upgraded, it will keep the existing alias if used by other dependencies. For example, some other dependency may need prettier@^2.1.1 (AKA some version of prettier >= v2.1.1). The underlying version 2.4.1 is what's actually referenced and used here, as you can see by the link to the prettier tarball in the yarn package registry

[ohltyler]

reffers -> refers. Also, can you add an example on what a possible exception could be? (security, failure reading the index, etc.).

[ohltyler]

LGTM!