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

Copy format from allOf in JSONSchemaBridge #863

Closed
hmvp opened this issue Jan 27, 2021 · 7 comments · Fixed by #872
Closed

Copy format from allOf in JSONSchemaBridge #863

hmvp opened this issue Jan 27, 2021 · 7 comments · Fixed by #872
Assignees
Labels
Type: Question Questions and other discussions

Comments

@hmvp
Copy link

hmvp commented Jan 27, 2021

The code copies properties and type from allOf refs:

_definition.required = definition.required ?? [];
combinedPartials.forEach(({ properties, required, type }) => {
if (properties) {
Object.assign(_definition.properties, properties);
}
if (required) {
_definition.required.push(...required);
}
if (type && !_definition.type) {
_definition.type = type;
}
});
}

However I have a schema like this:

{
                "title": "title",
                "type": "object",
                "properties": {
                    "bfd": {
                        "title": "Bfd",
                        "default": {
                            "enabled": false
                        },
                        "allOf": [
                            {
                                "$ref": "#/definitions/BFD"
                            }
                        ]
                    }
                },
                "definitions": {
                
                    "BFD": {
                        "title": "BFD",
                        "type": "object",
                        "properties": {
                            "enabled": {
                                "title": "Enabled",
                                "type": "boolean"
                            },
                            "minimum_interval": {
                                "title": "Minimum Interval",
                                "default": 900,
                                "type": "integer",
                                "minimum": 1,
                                "maximum": 255000
                            },
                            "multiplier": {
                                "title": "Multiplier",
                                "default": 3,
                                "type": "integer",
                                "minimum": 1,
                                "maximum": 255
                            }
                        },
                        "required": [
                            "enabled"
                        ],
                        "format": "optGroup"
                    }
                }
            }

And my code relies on the format property to choose the right component. However it is not copied from the BFD definition. I find it odd that only a few keys are copied and not everything. If anything I would like to see format copied, but maybe everything should be copied that does not exist yet.

@radekmie radekmie self-assigned this Jan 28, 2021
@radekmie radekmie added the Type: Question Questions and other discussions label Jan 28, 2021
@radekmie radekmie added this to Needs triage in Open Source (migrated) via automation Jan 28, 2021
@radekmie radekmie moved this from Needs triage to In progress in Open Source (migrated) Jan 28, 2021
@radekmie
Copy link
Contributor

Hi @hmvp. The problem with copying all properties is that they can overwrite each other. The reason why we merge properties and required is that (a) we know their structure (it's still possible to override a single property though), (b) it's required to support allOf/anyOf/oneOf at all. It's already not 100% correct for the type property, as only the first one is being used, so type unions do not work at all.

[...] but maybe everything should be copied that does not exist yet.

That's a valid request. I'm not sure about it though. Maybe it'd be better to rethink the handling of these properties? I mean, this approach (accumulating properties) makes sense for allOf, but not so much for anyOf and oneOf. Let us think about it and please do put your comment as well.

@hmvp
Copy link
Author

hmvp commented Jan 28, 2021

Yeah it seems fair that allOf, anyOf and oneOf need different handling.

My usecase involves schemas generated by pydantic: https://pydantic-docs.helpmanual.io/usage/schema/ Which afaik only uses allOf like in the example above..

So reading up a bit on the three keywords, some thoughts:

  • They are really meant for validation and allow unions. However validation is done by e.g. ajv and they can care about the proper way to do that.
  • We are more interested in the presentation, which widget to choose and with which properties. So it seems fair to combine them in a way..
  • I would say that it does not make sense to have more than one of the keywords. At least not from a validation perspective. What would be the meaning of "must validate to exactly oneOf X and at the same time to allOf X".
  • It makes sense to have multiple objects/references within a keywords which gives you type unions. But if its only one, like above it seems safe to just copy over everything. If there are more than that only makes a bit sense for allOf assuming this leads to the most constrained type and would not lead to conficts in which widget to show. Eg it makes sense to merge for a numeric widget a minimum from one object and a maximum from the other. It does not make sense to merge properties for a datewidget and a numeric widget..
  • Especially allOf seems to me more about class inheritance than about union types and in the former case you want to copy properties.
  • Way out of scope for this issue would be to solve the union types by adding a choice widget and depending on the value showing the widgets for type X or Y in the union. Again that makes most sense for oneOf and anyOf. In this case actually the would be no difference between anyOf an oneOf since you need to choose a object/reference anyway and thus will always satisfy oneOf even though anyOf is specified. Which is ok since oneOf is just a stronger version of anyOf
  • If there is somehow a conflict in propeties it is to the parent object to resolve it so we should not overwrite anything that is already present

@radekmie
Copy link
Contributor

I agree with most of what you've said.

Way out of scope for this issue would be to solve the union types by adding a choice widget and depending on the value showing the widgets for type X or Y in the union. [...]

It's out of scope of uniforms. This would require to let bridge know, what is the current state of the form (e.g. getField/getProps receive value or a whole model). And from my experience, it leads to unnecessary coupling. A fully working approach, achievable now, is to create a component and define it in the schema.

If there is somehow a conflict in propeties it is to the parent object to resolve it so we should not overwrite anything that is already present

Partially, it works like this now. But you have to adjust the schema yourself. And I think it's the suggested workaround for now - either adjust the schema manually (if you can) or create a function that does exactly that, based on your needs.

@hmvp
Copy link
Author

hmvp commented Jan 28, 2021

It's out of scope of uniforms. This would require to let bridge know, what is the current state of the form (e.g. getField/getProps receive value or a whole model). And from my experience, it leads to unnecessary coupling. A fully working approach, achievable now, is to create a component and define it in the schema.

It does not have to be. I would propose to do that in a stateless way. E.g. Similar to ListField the bridge would inject an intermediary field with a toggle widget and a hidden field for each option. This would probably mess with the paths a bit for those subfields...

{
"type": "object",
"properties": {
                    "union_field": {
                        "oneOf": [
                            { "type": "string" }, 
                            { "type": "int" }
                        ]
                    }
                }
}

would lead to something like

<AutoForm>
   <UnionField name="union_field">
      <ChooseTypeField name="union_field.$"/>
      <TextField name="union_field.0" />
      <NumField name="union_field.1"/>
   </UnionField>
</AutoForm>

where UnionField would need to do the heavy lifting to set the correct values in the model

Partially, it works like this now. But you have to adjust the schema yourself. And I think it's the suggested workaround for now - either adjust the schema manually (if you can) or create a function that does exactly that, based on your needs.
Yeah but given that there is not a good place for such a function in our use case. I still would like to see format and maybe other keys in the dict passed to the parent object similar to type..

@radekmie
Copy link
Contributor

Your example has one problem - how UnionField knows, which one to render? Let's take a nasty example:

{
  "type": "object",
  "properties": {
    "union_field": {
      "oneOf": [
        { "type": "int", "min": 5 }, 
        { "type": "int", "max": 5 }
      ]
    }
  }
}

And that's what I meant - you can create UnionField on a per-case basis. However, creating a general one is not possible (without validating the data).

@hmvp
Copy link
Author

hmvp commented Feb 1, 2021

Another similar issue:

{
	"form": {
		"type": "object",
		"properties": {
			"port_mode": {
				"default": "link_member",
				"allOf": [
					{
						"$ref": "#/definitions/PortModeChoice"
					}
				]
			}
		},
		"definitions": {
			"PortModeChoice": {
				"title": "PortModeChoice",
				"description": "An enumeration.",
				"enum": [
					"untagged",
					"tagged",
					"link_member"
				],
				"type": "string"
			}
		}
	}
}

enum and description are also not copied..

@radekmie
Copy link
Contributor

radekmie commented Feb 3, 2021

I've discussed this issue with a few more people heavily relying on JSON schema. To sum up, we all agree that the current handling of allOf/anyOf/oneOf is far from perfect and is there just to make it possible to render these forms at all. I am aware that there are uncovered cases, just like the one discussed here. However, I don't want to fix them while breaking others.

We'll update the documentation with limitations of the current implementation as well as example workarounds.

For this very problem, a workaround would be to preprocess the schemas before passing them to the form, hoisting certain properties from allOf to the property definition. I believe it's not a lot of code and has to implemented at most once per app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Question Questions and other discussions
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants