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

Add limited 'oneOf' json schema dependencies support for plan schemas. #677

Closed
wants to merge 1 commit into from
Closed

Add limited 'oneOf' json schema dependencies support for plan schemas. #677

wants to merge 1 commit into from

Conversation

david-martin
Copy link
Member

@david-martin david-martin commented Apr 30, 2018 •
edited

This change examines the json schema object in the service plan,
looking for a dependencies object.
If this exists, any oneOf property dependencies defined will be
examined and a corresponding angular-schema-form condition will
be added to the form definition so specified fields will only be
shown when the corresponding option is selected/entered in another
field.

Short clip of how it looks
https://youtu.be/MX-G0TjaP7E

Example plan

{
  "metadata": {
    "name": "5f04a3f508d7615a422863814f58ad98"
  },
  "spec": {
    "clusterServiceBrokerName": "ansible-service-broker", // some fields omitted
    "externalMetadata": {
      "schemas": {
        "service_binding": {},
        "service_instance": {
          "create": {
            "openshift_form_definition": [
              "eg_enum_param",
              "eg_conditional_one",
              "eg_conditional_two",
              "eg_boolean_param",
              "eg_conditional_boolean_one",
              "eg_conditional_boolean_two"
            ]
          },
          "update": {}
        }
      }
    },
    "instanceCreateParameterSchema": {
      "$schema": "http://json-schema.org/draft-04/schema",
      "additionalProperties": false,
      "dependencies": {
        "eg_boolean_param": {
          "properties": {
            "eg_conditional_boolean_one": {
              "title": "Example Shown When True",
              "type": "string"
            },
            "eg_conditional_boolean_two": {
              "title": "Example Also Shown When True",
              "type": "string"
            }
          },
          "required": [
            "eg_conditional_boolean_one",
            "eg_conditional_boolean_two"
          ]
        },
        "eg_enum_param": {
          "oneOf": [
            {
              "properties": {
                "eg_conditional_one": {
                  "title": "Example Shown If 'Yes' or 'Maybe'",
                  "type": "string"
                },
                "eg_enum_param": {
                  "enum": [
                    "Yes",
                    "Maybe"
                  ]
                }
              },
              "required": [
                "eg_conditional_one"
              ]
            },
            {
              "properties": {
                "eg_conditional_two": {
                  "title": "Example Shown if 'No'",
                  "type": "string"
                },
                "eg_enum_param": {
                  "enum": [
                    "No"
                  ]
                }
              },
              "required": [
                "eg_conditional_two"
              ]
            }
          ]
        }
      },
      "properties": {
        "eg_boolean_param": {
          "title": "Conditional Example Boolean",
          "type": "boolean"
        },
        "eg_enum_param": {
          "default": "Yes",
          "enum": [
            "Yes",
            "No",
            "Maybe"
          ],
          "title": "Conditional Example",
          "type": "string"
        }
      },
      "type": "object"
    } // some fields omitted
  }
}

Example parameters in an apb.yml (pending related broker changes in openshift/ansible-service-broker#928)

plans:
  - name: default
    description: __DEPENDENCIES
    free: True
    metadata: {}
    parameters:
    - name: eg_enum_param
      type: enum
      title: Conditional Example
      default: "Yes"
      enum: ["Yes", "No", "Maybe"]
    - name: eg_conditional_one
      title: Example Shown If 'Yes' or 'Maybe'
      type: string
      dependencies:
      - key: eg_enum_param
        value: ["Yes", "Maybe"]
    - name: eg_conditional_two
      title: Example Shown if 'No'
      type: string
      dependencies:
      - key: eg_enum_param
        value: "No"
    - name: eg_boolean_param
      title: Conditional Example Boolean
      type: boolean
    - name: eg_conditional_boolean_one
      title: Example Shown When True
      type: string
      dependencies:
      - key: eg_boolean_param
    - name: eg_conditional_boolean_two
      title: Example Also Shown When True
      type: string
      dependencies:
      - key: eg_boolean_param

Changes based on initial integration with broker (with @philipgough)

  • Allow for multiple condition matches i.e. the property enum array has more than 1 string
  • Auto add fields to the properties if they aren't already there i.e. it should be OK to define a property in a dependency only, rather than having to define it in the dependencies and in the properties

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 30, 2018
@david-martin
Copy link
Member Author

david-martin commented Apr 30, 2018 •
edited

ping @spadgett @philipgough @pb82 @maleck13

Related to UI & Broker changes discussed in openshift/ansible-service-broker#859 & #675

@maleck13
Copy link

maleck13 commented May 1, 2018

In the example I see a field always shows. Is it possible to not show any fields also? excuse my ignorance just not sure

@david-martin
Copy link
Member Author

david-martin commented May 1, 2018 •
edited

@maleck13 do you mean "don't show this field if another field has a specific value" ?

If yes, I don't think that's part of json schema dependencies. i.e. you define fields that are dependencies of others, but not fields that are negated dependencies.
You can workaround this though quite easily e.g

  • field 1 gives 3 options.
  • only show field 2 if option 1 is chosen
  • only show field 3 if option 2 is chosen
  • only show field 4 if option 3 is chosen

In this case, field 3 & 4 won't be shown if option 1 is selected on field 1.

Is there a use case you're thinking of?

@david-martin david-martin changed the title Add limited 'oneOf' json schema dependencies support for plan schemas. WIP: Add limited 'oneOf' json schema dependencies support for plan schemas. May 4, 2018
@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 4, 2018
@david-martin
Copy link
Member Author

david-martin commented May 8, 2018 •
edited

@philipgough I've added support for below if you want to try it again:

  • auto add properties defined in dependencies to the schema properties if they are not already there
  • allow matching from multiple strings

@david-martin
Copy link
Member Author

After discussion on openshift/ansible-service-broker#859 (comment) I'll look to round out this PR by allowing oneOf conditions with booleans and numbers (not just string).

@david-martin david-martin changed the title WIP: Add limited 'oneOf' json schema dependencies support for plan schemas. Add limited 'oneOf' json schema dependencies support for plan schemas. May 16, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 16, 2018
@david-martin
Copy link
Member Author

@spadgett This is ready for review now.
Where would be an appropriate place for documenting this catalog feature?

spadgett
spadgett reviewed May 16, 2018
View reviewed changes
src/components/catalog-parameters/catalog-parameters.controller.ts Outdated
_.each(requiredFields, (requiredField: any) => {
let fieldIndex = _.findIndex(this.ctrl.parameterForm, (field: any) => {
if (_.isObject(field)) {
return requiredFields.indexOf(field.key) > -1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

return _.includes(requiredFields, field.key);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is actually wrong.
It should be:

        if (_.isObject(field)) {
          return requiredField === field.key;
        } else {
          return requiredField === field;
        }

src/components/catalog-parameters/catalog-parameters.controller.ts Outdated

// Check if the required field is already in the schema properties.
// Add it if not.
if (_.isUndefined(this.ctrl.parameterSchema.properties[field.key])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably write this as

!_.has(this.ctrl.parameterSchema, ['properties', field.key]) {

src/components/catalog-parameters/catalog-parameters.controller.ts Outdated

private mapDependencyCondition(schema, key) {
// Get the fields to show if the dependency is valid
let requiredFields = _.get(schema, 'required');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not good about this in the catalog repo, but better to use const

src/components/catalog-parameters/catalog-parameters.controller.ts Outdated
}

// Set the angular 'condition'
let enumMatches = _.get(schema, 'properties.' + key + '.enum');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

const enumMatches = _.get(schema, ['properties', key, 'enum']);

src/components/catalog-parameters/catalog-parameters.controller.ts Outdated

// Set the angular 'condition'
let enumMatches = _.get(schema, 'properties.' + key + '.enum');
if (_.isUndefined(enumMatches)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be...?

if (enumMatches) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an undefined check, so do you mean if (!enumMatches) { ?

src/components/catalog-parameters/catalog-parameters.controller.ts Outdated

private escapeIfString(value) {
if (_.isString(value)) {
return '"' + value + '"';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so if I understand correctly this is escaping the string to use in an Angular expression. If the string contains an " character, could I break out an execute arbitrary expressions?

Should we just use JSON.stringify on all values?

What types can value be?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so if I understand correctly this is escaping the string to use in an Angular expression. If the string contains an " character, could I break out an execute arbitrary expressions?

I have tested a value with quotes in it (in the serviceplan definition), and they are escaped before they get to this point.
However, ...

Should we just use JSON.stringify on all values?

... this makes a lot of sense and will definitely prevent arbitrary expressions.
I'll change it to use this.

What types can value be?

String, number, boolean.

src/components/catalog-parameters/catalog-parameters.controller.ts Outdated
test: this.escapeIfString(enumMatches[0])
});
// Allow matching with any other strings in the array (if there are multiple)
for (var oi=1, ol=enumMatches.length; oi<ol; oi++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let

@david-martin
Copy link
Member Author

@spadgett Thanks for the feedback. My typescript skills are obviously lacking :)

I've pushed up changes for re-review

spadgett
spadgett approved these changes May 21, 2018
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but hold for 3.11

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2018
@spadgett spadgett added this to the 3.11.0 milestone May 21, 2018
@david-martin
Add limited 'oneOf' json schema dependencies support for plan schemas.
271a12e 
This change examinse the json schema object in the service plan,
looking for a `dependencies` object.
If this exists, any `oneOf` property dependencies defined will be
examined and a corresponding angular-schema-form `condition` will
be added to the form definition so specified fields will only be
shown when the corresponding option is selected/entered in another
field.

Example service plan that has a `oneOf` dependency for changing the
follow up fields depending on the selected value in the first selection
field.
https://gist.github.com/david-martin/594464b8193fb3f81c9bd878709731b5
@maleck13
Copy link

@spadgett ok to merge this now as it is generically useful

@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 23, 2020
@david-martin
Copy link
Member Author

Out of date, no longer required

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett approved these changes

Assignees
No one assigned
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
3.11.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@david-martin @maleck13 @openshift-bot @spadgett @openshift-ci-robot