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.

Sign up for GitHub

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

Jump to bottom

fix: merge schema properties with definitions $ref #3604

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: merge schema properties with definitions $ref #3604

wants to merge 1 commit into from

Conversation

ThibaudAV
Copy link

@ThibaudAV ThibaudAV commented Feb 25, 2023 •
edited

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • Remove some duplicated code
  • Corrects properties with the use of a definition

to have the same behavior as here https://rjsf-team.github.io/react-jsonschema-form/

Try with :

{
          definitions: {
            address: {
              properties: {
                address1: { type: 'string', title: 'Address 1' },
              },
            },
          },
          type: 'object',
          properties: {
            billing_address: {
              properties: {
                customAddress: { type: 'string', title: 'Custom address' },
              },
              $ref: '#/definitions/address',
            },
          },
        }

What is the current behavior? (You can also link to an open issue here)

Only add definitions properties

What is the new behaviour (if this is a feature change)?

Merge add definitions properties and parent one

Please check if the PR fulfills these requirements

Please provide a screenshot of this feature before and after your code changes, if applicable.

Other information:

@ThibaudAV ThibaudAV marked this pull request as ready for review February 25, 2023 09:14
@netlify
Copy link

netlify bot commented Feb 25, 2023

✅ Deploy Preview for formly-dev ready!

Name Link
🔨 Latest commit 2e8cdb7
🔍 Latest deploy log https://app.netlify.com/sites/formly-dev/deploys/63f9d0dff1d8eb00085d027f
😎 Deploy Preview https://deploy-preview-3604--formly-dev.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 settings.

@kenisteward
Copy link
Collaborator

kenisteward commented Feb 25, 2023 via email

@kenisteward
Copy link
Collaborator

kenisteward commented Feb 25, 2023 via email

@kenisteward
Copy link
Collaborator

kenisteward commented Feb 25, 2023 via email

@ThibaudAV
Copy link
Author

I wanted to test with an allOf but it gives me a result that is not what I expected 😁

Json Schem 
{
"type": "object",
"definitions": {
  "userType": {
    "oneOf": [
      {
        "title": "Human",
        "properties": {
          "pseudo": {
            "type": "string",
            "title": "Pseudo"
          }
        },
        "required": [
          "pseudo"
        ]
      },
      {
        "title": "Robot",
        "properties": {
          "serialNumber": {
            "type": "string",
            "title": "Serial number"
          }
        },
        "required": [
          "serialNumber"
        ]
      }
    ]
  },
  "authMethod": {
    "oneOf": [
      {
        "title": "With token",
        "properties": {
          "token": {
            "type": "string",
            "title": "Token"
          }
        },
        "required": [
          "token"
        ]
      },
      {
        "title": "With username and password",
        "properties": {
          "username": {
            "type": "string",
            "title": "Username"
          },
          "password": {
            "type": "string",
            "title": "Password"
          }
        },
        "required": [
          "username"
        ]
      }
    ]
  }
},
"properties": {
  "allOfWithProperties": {
    "title": "AllOf with properties",
    "type": "object",
    "allOf": [
      {
        "properties": {
          "first": {
            "title": "First",
            "type": "string"
          }
        }
      },
      {
        "$ref": "#/definitions/authMethod"
      },
      {
        "properties": {
          "second": {
            "title": "Second",
            "type": "string"
          }
        }
      },
      {
        "$ref": "#/definitions/userType"
      }
    ]
  }
}
}

I would have expected to have all fiels and multishema in the allOf order. But apparently the AllOf with OneOf not worked as i expect. Only one "OneOf" are displayed and not in the right order.

@ThibaudAV
Copy link
Author

ThibaudAV commented Feb 28, 2023 •
edited

I close it.
I hope to find other solutions for my use case. If you have suggestions, do not hesitate 🙂

@ThibaudAV ThibaudAV closed this Feb 28, 2023
@kenisteward
Copy link
Collaborator

kenisteward commented Mar 1, 2023 via email

@ThibaudAV
Copy link
Author

Or course : https://stackblitz.com/edit/angular-jumm2x?file=src%2Fapp%2Fapp.component.ts,src%2Fapp%2Fapp.component.html

I would expect to have the output fields in the order :

  • First field
  • authMethod ref fields
  • Second field
  • userType ref fields

Thank you for your help 🙏

@kenisteward
Copy link
Collaborator

kenisteward commented Mar 2, 2023 via email

@aitboudad
Copy link
Member

I'll revised this once time allows 🙏, have you tried adjusting the order through the map option #1982 (comment)

@aitboudad aitboudad reopened this Mar 3, 2023
@kenisteward
Copy link
Collaborator

@ThibaudAV what @aitboudad does fix the ordering issue! I forgot this was added in order to define what order the fields which fixed that.

The other issue of logic for merging oneOf's seems like it's a bigger discussion ticket though.

In the context of validations it makes sense because jsonschema is just a validation language that parses them disparately as separate rules. When it comes to generating fields though we have to decide if we merge them what exactly happens. Right now it looks like first one wins ( or last one I can't remember haha )

@ThibaudAV
Copy link
Author

@kenisteward

If they are on disparate keys it looks like it works great
https://stackblitz.com/edit/angular-jumm2x-jevk23?file=src%2Fapp%2Fapp.component.ts,src%2Fapp%2Fapp.component.html

This changes the structure of the object and I can't change that

No the order does not fix my problem :/

And yes for OneOf merger. Maybe there is something to change on this side. maybe add them in a respective sub form. or something like that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ThibaudAV @kenisteward @aitboudad