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

Valid JSON Schema with Dot-Fieldnames breaks JSONSchemaBridge 3.5.1 #963

Closed
rk3rn3r opened this issue May 19, 2021 · 3 comments · Fixed by #1086
Closed

Valid JSON Schema with Dot-Fieldnames breaks JSONSchemaBridge 3.5.1 #963

rk3rn3r opened this issue May 19, 2021 · 3 comments · Fixed by #1086
Assignees
Labels
Type: Feature New features and feature requests
Milestone

Comments

@rk3rn3r
Copy link

rk3rn3r commented May 19, 2021

Hey!

We are using uniforms with uniforms-bridge-json-schema/JSONSchemaBridge.
We have a valid JSON Schema, but the schema is for a tool that has dots in the field/property names like mystructure.value.enabled.
Unfortunately JSONSchemaBridge seems to expect that dots in names are addressing a hierarchy instead of just a plain name with dots. That expectation might be true from an HTML form view, but is not valid for JSON/JSON Schema. So we have a valid schema but can't convert it successfully with AutoForm.
We receive a Field not found in schema: "mystructure.value.enabled" error.

I expect only when you traverse nested objects in the JSON Schema you should expect a nested structure, but not base this on property names.

Or is there anything I don't see? Help would be really appreciated.

@radekmie radekmie self-assigned this May 19, 2021
@radekmie radekmie added the Type: Bug Bug reports and their fixes label May 19, 2021
@radekmie radekmie added this to the v3.x milestone May 19, 2021
@radekmie
Copy link
Contributor

Hi @rk3rn3r! Let's make this one in bold: I really hoped that no one will ever ask. I have a long story to tell here and no time to do so... Let's make it quick then.

Since the beginning, uniforms heavily depend on the fact that the . is the name separator. It is the case for SimpleSchema, which was the only bridge implemented at the start. It even works for GraphQL, as a dot is not a valid field name. But then there's JSON schema.

By looking at the current architecture, the only place that actually cares about it is the joinName helper and the _.get/_.set/_.setWith family. Theoretically, we could handle it somehow within the joinName, but the rest would require additional work. I'm not saying it's impossible or requires a breaking change, but it's definitely not easy.

I believe that a subset of JSON Path or JSON Pointer would be the best. The simplest extension that would be enough to cover your case is to accept paths with escapes, e.g., section.['escaped.name'].0. Some ground work is already there to cover all cases of the error handling.

As for now, the only workaround is to translate the schema to replace these dots with another character. If you need help with that, do let us know here. If you have ideas or other comments - please do share them.

@rk3rn3r
Copy link
Author

rk3rn3r commented May 19, 2021

Thx a lot for your reply @radekmie!

Like I said in the initial description, I can understand, when thinking about this naming/hierarchies from HTML side, especially forms, the hierarchy actually makes sense.

But if this is reflected in the internal structure of uniforms it is unfortunate, especially because the constraint that the dot is a hierarchy separator does not make sense for Javscript/JSON where a member/property name like mystructure.value.enabled might be invalid, but as string "mystructure.value.enabled" it's valid and many APIs make use of that in their JSON.

I see that this might cause trouble and I am very glad that you still consider this for a milestone! 🙏

Thx a lot for the pointers and my colleagues & I will have a look if we can temporarily quick-fix this for us with the help of that guidance, and maybe contribute a better and complete fix later. But I can't promise!

FYI @riccardo-forina @mdrillin @uidoyen @indraraj @wtrocki

@kestarumper kestarumper self-assigned this Nov 19, 2021
@radekmie
Copy link
Contributor

(We've had a team discussion about this issue, and here are the notes.)

  • Current behavior:
    • joinName('a', 'b') === 'a.b'.
    • joinName('a', "'b") === "a.'b".
    • joinName('a', '"b') === 'a."b'.
    • joinName('a', 'b.c') === 'a.b.c'.
    • joinName(null, 'a', 'b') === ['a', 'b'].
    • joinName(null, 'a', "'b") === ['a', "'b"].
    • joinName(null, 'a', '"b') === ['a', '"b'].
    • joinName(null, 'a', 'b.c') === ['a', 'b', 'c'].
    • joinName(joinName(null, ...xs)) === joinName(...xs) <- we have to preserve this.
    • joinName(null, joinName(...xs)) === joinName(null, ...xs) <- we have to preserve this.
  • Idea 1: use JSONPath-like approach with brackets.
    • joinName('a', 'b.c') === "a.b.c" <- expected.
    • joinName('a', '"b.c"') === "a['b.c']" <- expected.
    • joinName('a', "'b.c'") === "a['b.c']" <- expected.
    • joinName(null, 'a', 'b.c') === ['a', 'b', 'c'] <- expected.
    • joinName(null, 'a', '"b.c"') === ['a', '"b.c"'] <- expected.
    • joinName(null, 'a', "'b.c'") === ['a', '"b.c"'] <- expected.
    • myobject[last(joinName(null, name))] <- this will break for 'b.c', because the key is 'b.c'.
    • It also breaks compatibility for fields with " and ' in it.
  • Idea 2: provide a list-based implementation, where all singular strings are split on dots and lists are treated as they are.
    • joinName('a', 'b.c') === 'a.b.c' <- expected?
    • joinName('a', ['b.c']) === "a['b.c']" <- expected?
    • joinName(null, 'a', 'b.c') === ['a', 'b', 'c'] <- expected?
    • joinName(null, 'a', ['b.c']) === ['a', 'b.c'] <- expected?
    • The only thing that breaks is the usage of arrays in joinName.
    • Everyone who is now doing things like <AutoField name="x.y" /> will have to change it to <AutoField name={['x.y']} />.
    • It's fine as having dots in your schema field names AND handcrafting a form at the same time is very rare.
    • Idea 2.5:
      • Alternatively to name={['x.y']} is a special string format, like name="__I_HAVE_DOTS__x.y".
  • Idea 3: never serialize names at all and always pass objects around.
    • joinName('a', ['b.c']) === ['a', 'b.c'] <- expected?
    • joinName(null, 'a', ['b.c']) === ['a', 'b.c'] <- expected?
  • Idea 4: create more functions.
    • splitNames("a['b.c']") === ['a', 'b.c'] <- expected.
    • concatNames('a', 'b.c') === ['a', 'b.c'] <- expected.
    • serializeNames(['a', 'b.c']) === "a['b.c']" <- expected.
  • Idea 5: we use JSON.stringify or, more generally, "you can't assume how the string will look like".
    • joinName('a', 'b.c') === 'some-string-that-we-understand' <- expected?
    • joinName(null, 'a', 'b.c') === ['a', 'b.c'] <- expected?
  • Idea 6: like 2 but with a special wrapper instead of arrays.
    • joinName('a', 'b.c') === "a.b.c" <- expected.
    • joinName('a', Wrapper('b.c')) === "a['b.c']" <- expected.
    • joinName(null, 'a', 'b.c') === ['a', 'b', 'c'] <- expected.
    • joinName(null, 'a', Wrapper('b.c')) === ['a', Wrapper('b.c')] <- expected.

Idea 2 (or 2.5) seems to be the best and the easiest to use. @kestarumper will continue his research of this topic.

@radekmie radekmie added Type: Feature New features and feature requests and removed Type: Bug Bug reports and their fixes labels Dec 10, 2021
@radekmie radekmie removed their assignment Dec 10, 2021
@radekmie radekmie modified the milestones: v3.x, v4.0 Dec 10, 2021
@radekmie radekmie linked a pull request Feb 3, 2022 that will close this issue
@radekmie radekmie modified the milestones: v4.0, v3.x Feb 3, 2022
@radekmie radekmie assigned radekmie and unassigned kestarumper Feb 3, 2022
@radekmie radekmie modified the milestones: v3.x, v3.8 Apr 22, 2022
@radekmie radekmie moved this to Closed in Open Source Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features and feature requests
Projects
Archived in project
3 participants