isRequired and oneOf interaction

General
1

I wrote a test with this expectation

test('isRequired - should return true if property is required', t => {
  const rootSchema = {
    "type": "object",
    "properties": {
      "foo": {
        "oneOf": [
          { type: "number" },
          { type: "string" }
        ]
      }
    },
    "required": ["foo"]
  }
  const schema = { type: "number" }
  const schemaPath = "#/properties/foo"
  t.is(isRequired(schema, schemaPath, rootSchema), true)
})

but isRequired returns false.

Is this expected behaviour or would you accept a PR for this behaviour?

[original thread by Lily]

2

Hi @lilyh, thanks for the question :wink:

I’m assuming you mean the the non-exported isRequired method in renderer.ts file. It’s meant to be called with a schema as the first parameter which contains the resolvable path. So the test in your code should look like this: t.is(isRequired(rootSchema, schemaPath, rootSchema), true) which should pass.

The rootSchema parameter is used as a fallback in cases where the method is given a subschema of the rootschema as the first parameter and the schemaPath is also correctly scoped for that subschema, however the subschema contains a $ref pointer (or consists only of one) which we have to follow which points outside of the subschema and therefore has to be resolved against the rootSchema. Note that the ref-resolving code evolved over time so I’m not sure if there is currently even a case where isRequired is called with such a non-resolved schema.

In short: Yes the behavior is as expected :wink:

3

[Lily]

4

[Lily]

Hey thanks @sdirix

I’ve created this minimal reproduction of the issue I’m having
https://github.com/HelixCentre/jsonforms-react-seed

const FooControl = ({
  data,
  handleChange,
  path,
  errors,
  required
}: ControlProps) => (
  <div>
    <p>The value of required is: {JSON.stringify(required)}</p>
    <p>Errors: {JSON.stringify(errors)}</p>
  </div>
);

export default withJsonFormsControlProps(FooControl);

I think it might be an issue that the control has "“is a required property” error from AJV but the value of required is false.

The test above was intended to match this scenario but perhaps is inaccurate.

5

Good find! I agree that the control should know it is required.

As the isRequired method can’t handle subschemas as outlined above, we could either adjust it or the mapStateToControlProps to take these cases into account. However instead of adding yet another special case I would prefer to simplify the functionality in general. Ideally we should refactor the isRequired method to only expect the root schema and the full schema path. This way we would avoid all the complications with subschemas and non-resolvable paths etc.

Note that we already added a refactoring like that (+ many other changes) in Async ref resolving by edgarmueller · Pull Request #1505 · eclipsesource/jsonforms · GitHub. However it might take some time until this is merged to master. If you’d like to create a PR only for simplifying the isRequired method and its callers including some testcases we would merge that first and rebase the refactoring on that.