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

Failure to render with "additionalProperties": True #22

Open
thclark opened this issue Apr 30, 2021 · 2 comments
Open

Failure to render with "additionalProperties": True #22

thclark opened this issue Apr 30, 2021 · 2 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@thclark
Copy link

thclark commented Apr 30, 2021

  • django-reactive version: 0.0.10
  • Django version: 3.1.8
  • Python version: 3.8.6
  • Operating System: ubuntu buster (python buster-slim container, running on OSX via docker),

Description

First, thanks for the library. It seems like early stages but this has the potential to become a n elegant solution to a lot of problems storing semi structured data in django+postgres.

I attempted to use a schema with the additionalProperties attribute set to True. This causes the form (in this case, for the 'tags' field) not to be rendered in django admin:

Screenshot 2021-04-30 at 12 36 28

Because JSON schema are able to use $REFs, it may not always be possible to force additionalProperties to false - and it's certainly not desirable, because it's good to be able to use the schema.

What I Did

I got the documented example to work and render the form in django admin.

Then I attempted to use the following schema:

Fails to render form

    tags = ReactJSONSchemaField(
        schema={
            "title": "Fails with additionalProperties True",
            "type": "object",
            "required": [
                "timezone",
            ],
            "properties": {
                "timezone": {
                    "type": "string",
                    "title": "Timezone",
                    "description": "Because met mast data files don't include timezone. Sigh!"
                },
            },
            "additionalProperties": True
        }
    )

The console output in chrome devtools shows:

**react-dom.js?Expires=1619868979&GoogleAccessId=stuff TypeError: Cannot convert undefined or null to object**
    at keys (<anonymous>)
    at utils.js:541
    at oe (utils.js:600)
    at B (utils.js:260)
    at r.value (Form.js:65)
    at new r (Form.js:35)
    at Yg (react-dom.js?Expires=1619868979&GoogleAccessId=stuff:68)
    at rh (react-dom.js?Expires=1619868979&GoogleAccessId=stuff:98)
    at zj (react-dom.js?Expires=1619868979&GoogleAccessId=stuff:228)
    at Th (react-dom.js?Expires=1619868979&GoogleAccessId=stuff:152)
Me @ react-dom.js?Expires=1619868979&GoogleAccessId=stuff:125
Ih.c.callback @ react-dom.js?Expires=1619868979&GoogleAccessId=stuff:138
Wg @ react-dom.js?Expires=1619868979&GoogleAccessId=stuff:67
oj @ react-dom.js?Expires=1619868979&GoogleAccessId=stuff:127
Aj @ react-dom.js?Expires=1619868979&GoogleAccessId=stuff:160
unstable_runWithPriority @ react.js?Expires=1619868979&GoogleAccessId=stuff:25
Da @ react-dom.js?Expires=1619868979&GoogleAccessId=stuff:60
ab @ react-dom.js?Expires=1619868979&GoogleAccessId=stuff:154
Te @ react-dom.js?Expires=1619868979&GoogleAccessId=stuff:146
Ja @ react-dom.js?Expires=1619868979&GoogleAccessId=stuff:224
md @ react-dom.js?Expires=1619868979&GoogleAccessId=stuff:173
(anonymous) @ react-dom.js?Expires=1619868979&GoogleAccessId=stuff:175
Rh @ react-dom.js?Expires=1619868979&GoogleAccessId=stuff:147
nd @ react-dom.js?Expires=1619868979&GoogleAccessId=stuff:175
I.render @ react-dom.js?Expires=1619868979&GoogleAccessId=stuff:238
djangoReactiveRenderForm @ django_reactive.js?Expires=1619868979&GoogleAccessId=stuff:76
(anonymous) @ (index):518

**react-dom.js?Expires=1619868979&GoogleAccessId=stuff:161 Uncaught TypeError: Cannot convert undefined or null to object**
    at keys (<anonymous>)
    at utils.js:541
    at oe (utils.js:600)
    at B (utils.js:260)
    at r.value (Form.js:65)
    at new r (Form.js:35)
    at Yg (react-dom.js?Expires=1619868979&GoogleAccessId=stuff:68)
    at rh (react-dom.js?Expires=1619868979&GoogleAccessId=stuff:98)
    at zj (react-dom.js?Expires=1619868979&GoogleAccessId=stuff:228)
    at Th (react-dom.js?Expires=1619868979&GoogleAccessId=stuff:152)

(don't worry about google access id stuff, that's just the bucket I serve my static assets from)

By systematically comparing various missing / extra bits compared to the given example, I got the following schema to work:

Works:

    tags = ReactJSONSchemaField(
        schema={
            "title": "Works with additionalProperties False",
            "type": "object",
            "required": [
                "timezone",
            ],
            "properties": {
                "timezone": {
                    "type": "string",
                    "title": "Timezone",
                    "description": "Because met mast data files don't include timezone. Sigh!"
                },
            },
            "additionalProperties": False  # Also works if omitted, since False is default
        }
    )

Suggestion

Whilst it's certainly possible, it's a big undertaking to create a form which will allow on-the-fly creation of additionalProperties. So, maybe you don't actually want to do that. But even if you're not providing the scope for creating that additional data in django admin, I think the widget needs to not crash if the schema allows for it.

That way, the schema that you use for form generation can be made reusable.

@tyomo4ka
Copy link
Member

tyomo4ka commented May 3, 2021

@thclark

Thanks for such a detailed ticket. I really appreciate the feedback and the time you put into it.

Definitely, the usage of additionalProperties is not supported in the library at the moment and we mention this in the README: https://github.com/CoverGenius/django-reactive#limitations. I agree, the library should fail it more gracefully though. I'm sorry for the time you spent to debug this problem.

This feature is something that we don't need at the moment at CG. But I agree, there are some good use cases for it, so I would like to implement support for it if it's possible.

Unfortunately, it's unlikely I will be able to work on it in the next month or longer due to some personal circumstances. But if you decide to work on a solution yourself, I and the team will be more than happy to merge it. Please, ping me, @jordaneremieff, @WenminZhao or @rabbit-aaron if you need some help with this. One of us should be here to review your code and release a new version. Otherwise, I will try to dedicate some time later in the year to work on this ticket.

@tyomo4ka tyomo4ka added enhancement New feature or request help wanted Extra attention is needed labels May 3, 2021
@thclark
Copy link
Author

thclark commented May 3, 2021

Thanks @tyomo4ka - sorry, I'd obviously not absorbed that limitation in the docs, just got the example and went for it!

I'm suffering from long covid myself, and struggling to make it through a work day, so probably won't get the energy to make a PR for this. But will definitely keep an eye on the project, keep it up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants