label-studio icon indicating copy to clipboard operation
label-studio copied to clipboard

fix: LEAP-634: Actualize label config validation

Open Gondragos opened this issue 1 year ago • 6 comments

Config validation didn't take into account many tags and their attributes. This PR might fix this.

It also fixes nested Choice validation.

It also will fix validation messages. (Usually there is no path at all and sometimes it doesn't tell about the problem itself)

PR fulfills these requirements

  • [ ] Tests for the changes have been added/updated
  • [ ] Docs have been added/updated
  • [x] Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • [x] Self-reviewed and ran all changes on a local instance

Explain the changes you've made

  1. Changing exc.path into exc.absolute_path fixes displaying validation paths from Validation failed on : to Validation failed on View/Labels/Label: .
  2. Changing
{
  "anyOf": [
    {
      "type": "array",
      "items": {"$ref": "#/definitions/ref1"}
    }, 
    {
      "$ref": "#/definitions/ref1"
    }
  ]
}  

into

{
  "anyOf": [
    {
      "$ref": "#/definitions/ref1"
    },
    {
      "type": "array",
      "items": {"$ref": "#/definitions/ref1"}
    } 
  ]
}  

fixes displaying cause of validation fails from OrderedDict() is not of type 'array' to something more meaningful like 'value' is a required property

  1. The line
# config = _fix_choices(config)

was hidden under the comment due to the problem, which caused this workaround, was solved by json schema itself.

What alternative approaches were there?

Fixing displayed paths has leaded to the situation when

<View>
  <Text name="text" value="$text" />
  <Choices name="choice" toName="choice">
    <Choice />
    <Choice value="any" />
  </Choices>
</View>

shows View/Choices/Choice/0, but

<View>
  <Text name="text" value="$text" />
  <Choices name="choice" toName="choice">
    <Choice />
  </Choices>
</View>

shows View/Choices/Choice. And previously it was just Choice/0 in this case.

To make it more consistent we can use removed right now function _fix_choices but convert all tags into arrays and in the same time get rid of MaybeMultiple* rules in schema. But so far it doesn't look to me that it's necessary, 'cause doesn't really affects user's navigation.

This change affects (describe how if yes)

  • [ ] Performance
  • [ ] Security
  • [ ] UX

Does this PR introduce a breaking change?

  • [ ] Yes, and covered entirely by feature flag(s)
  • [ ] Yes, and covered partially by feature flag(s)
  • [ ] No
  • [x] Not sure (briefly explain the situation below)

It depends on the validity of the validation rules.

What level of testing was included in the change?

  • [ ] e2e (codecept)
  • [ ] integration (cypress)
  • [ ] unit (jest)

Which logical domain(s) does this change affect?

Labeling Interface

Gondragos avatar Jan 19 '24 17:01 Gondragos

Deploy Preview for heartex-docs ready!

Name Link
Latest commit 4c0521b3682e6b304e4e169f3de2bc8bd3a8d6c2
Latest deploy log https://app.netlify.com/sites/heartex-docs/deploys/65b9c4c175233900080212d5
Deploy Preview https://deploy-preview-5313--heartex-docs.netlify.app/guide/billing
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Jan 19 '24 17:01 netlify[bot]

Deploy Preview for label-studio-docs-new-theme ready!

Name Link
Latest commit 4c0521b3682e6b304e4e169f3de2bc8bd3a8d6c2
Latest deploy log https://app.netlify.com/sites/label-studio-docs-new-theme/deploys/65b9c4c13df3b30008064e90
Deploy Preview https://deploy-preview-5313--label-studio-docs-new-theme.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Jan 19 '24 17:01 netlify[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 75.79%. Comparing base (4e0f517) to head (4c0521b). Report is 326 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #5313      +/-   ##
===========================================
- Coverage    75.88%   75.79%   -0.10%     
===========================================
  Files          154      154              
  Lines        12931    12930       -1     
===========================================
- Hits          9813     9800      -13     
- Misses        3118     3130      +12     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jan 22 '24 16:01 codecov[bot]

I will say we probably don't want to check in a commented-out function call; one way forward would be to remove the line and add a comment like

# previously we called `_fix_choices` here, but no longer need to because...

@jombooth Thanks for that! It looks much better.

Gondragos avatar Jan 31 '24 03:01 Gondragos

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

robot-ci-heartex avatar Jun 10 '24 21:06 robot-ci-heartex