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

Additional flexibility for imperativeConfiguration #56

Open
nickevansuk opened this issue Jan 22, 2020 · 6 comments
Open

Additional flexibility for imperativeConfiguration #56

nickevansuk opened this issue Jan 22, 2020 · 6 comments
Assignees

Comments

@nickevansuk
Copy link
Contributor

Requirement

Add additional flexibility to imperativeConfiguration to allow for the containing property name to be considered when setting required fields for a type.

This functionality would mean that, for example, validating the below Organization type would depend on the property within which it was found - e.g. "broker".

"broker": {
  "@type": "Organization",
  "name": "Acme leisure"
}

Suggested design

Use imperativeConfigurationWithContext within the model files, which includes a map within requiredFields, recommendedFields, shallNotInclude and requiredOptions. This is an additional map of property names (see example below). These changes already exist dormantly in Organization.json and Person.json - just remove the placeholder empty "imperativeConfiguration".

Implementation hints

Implementation of this likely requires changes to src/rules/core/required-optional-fields-rule.js, src/rules/core/required-fields-rule.js, src/rules/core/recommended-fields-rule.js, and src/rules/core/shall-not-include-fields-rule.js within data-model-validator.

Example

  "imperativeConfigurationWithContext": {
    "request": {
      "requiredFields": {
        "broker": [
          "id",
          "name"
        ],
        "seller": [
          "id"
        ],
        "customer": [
          "email",
          "name",
          "address"
        ]
      },
      "recommendedFields": {
        "broker": [
          "email",
          "url",
          "logo"
        ],
        "seller": [
        ],
        "customer": [
        ]
      }
    },
    "response": {
      "requiredFields": {
        "broker": [
        ],
        "seller": [
          "id",
          "name",
          "legalName",
          "taxMode",
          "vatID",
          "address"
        ],
        "customer": [
        ]
      },
      "recommendedFields": {
        "broker": [
        ],
        "seller": [
          "email",
          "url",
          "logo"
        ],
        "customer": [
        ]
      }
    }
  },
@nickevansuk nickevansuk changed the title Additional flexibility for imperativeConfiguration Validator: Additional flexibility for imperativeConfiguration Jan 27, 2020
@nickevansuk nickevansuk changed the title Validator: Additional flexibility for imperativeConfiguration Additional flexibility for imperativeConfiguration Jan 27, 2020
@henryaddison
Copy link
Contributor

henryaddison commented Feb 13, 2020

Trying to get my head around this and the validator code that would allow it. Am wondering if it could be handled using specific Rule classes in the validator rather than generalized rules relying on the config of the model? Because the validations desired depend on the containing object which the model itself doesn't need to know about (i.e. an object is being validated based on the way it is used, rather than based on the sort of object it is and values of the properties it has). Or alternatively by creating sub-models to specialize models for particular use-cases? e.g. to define specific sorts of Organizations like Brokers and Providers.

Having said all that, if it's desirable to keep the various definitions of what is valid in the data-models repo, perhaps some sort of DSL could be extracted to specify these sorts of validations rules in JSON that could then be run by generic Rules in the validator.

But maybe I'm overthinking it.

@henryaddison henryaddison self-assigned this Feb 18, 2020
@nickevansuk
Copy link
Contributor Author

Sorry @henryaddison have just seen this!

So the modelling approach taken in the specification means that defining a more specific type only happens when the semantics of the thing itself change (i.e. an OrderQuote is different to an Order), rather than just because a type is being used in a different context (i.e. an Organization is still an Organization regardless of whether it's used as a seller or customer, and indeed Person can be used in both of these cases).

The requirement here really comes from the spec itself:
Screenshot 2020-02-18 at 12 46 24

Is the main issue that the Model doesn't give access to the containing property? How are you planning to tackle this currently?

@henryaddison
Copy link
Contributor

Just started this morning and I'm going with your suggestion. I believe each rule is told the name of the containing field as well as the data and model being validated so hopefully it's straightforward to implement.

@nickevansuk
Copy link
Contributor Author

Ok great!

@henryaddison
Copy link
Contributor

@nickevansuk - would you mind if I swap the order of the property name and the modality fields? So for example with Organization it would be imperativeConfigurationWithContext.request.broker.{requiredFields, shallNotInclude, recommendedFields} rather than imperativeConfigurationWithContext.request.{requiredFields, shallNotInclude, recommendedFields}.broker. It just means that the object in, say, imperativeConfigurationWithContext.request.broker looks like an object in imperativeConfiguration.request so the code accessing the different possible locations is a little more consistent. I'm happy to go and change the Person and Organization models in the data-models repo.

@nickevansuk
Copy link
Contributor Author

Sure @henryaddison sounds good to me!

@nickevansuk nickevansuk transferred this issue from openactive/openactive-test-suite Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants