feat: flatten allOf cases in message payloads #16

jamescrowley · 2020-04-26T23:22:13Z

Description

This PR flattens usage of 'allOf' in message payloads. Usage of 'additionalProperties' can be problematic when using allOf, as it requires all provided schemas to successfully validate a message. See https://json-schema.org/understanding-json-schema/reference/combining.html#allof.

Therefore, most scenarios other than when 'additionalProperties' are permitted in every schema are likely to be edge cases as far as I can tell. Feedback especially welcome on this issue.

Note: I have based this off the changes in #13 for now

Related issue(s)

(Attempts) to Fix asyncapi/asyncapi-react#596

jamescrowley · 2020-04-26T23:25:10Z

filters/all.js

-
+
+  Nunjucks.addFilter('additionalPropertiesAllowedForAll', (items) => items.every(p => p.additionalProperties() === true || p.additionalProperties() === undefined || p.additionalProperties() === null));
+  Nunjucks.addFilter('additionalPropertiesDisabledForAll', (items) => items.every(p => p.additionalProperties() === false) );


I couldn't find a nice way to in-line this within the template. Suggestions welcome!

jamescrowley · 2020-04-26T23:26:21Z

test/spec/asyncapi.yml

@@ -145,6 +145,7 @@ components:
          x-pi: false
        sentAt:
          $ref: "#/components/schemas/sentAt"
+      additionalProperties: false


There are currently no allOf usages in the template. I wasn't sure on the best example to add here.

maybe take something from this https://github.com/asyncapi/tck/blob/master/tests/asyncapi-2.0/Schema%20Object/valid-polymorphism.yaml

don't worry it has nothing to do with streetlight example, we anyway need to one day prepare one dummy example that can be used in all the templates, that cover all cases. Other solution might be we make it possible to use tck as the library, maybe

derberg · 2020-04-27T20:12:12Z

@jamescrowley How should I treat https://github.com/asyncapi/html-template/pull/13/files? as it is correct and I can merge, but you also did the same changes in this PR

jamescrowley · 2020-04-27T23:49:32Z

@derberg I based this PR off that one, so if you're happy with #13 merge that first and I'll rebase - and if you're not, then I'll remove the changes :)

derberg · 2020-04-28T05:24:43Z

@jamescrowley lgtm, I just adjusted the PR summary as we use conventional commits. You can rebase this PR

In the case of 'allOf' schema elements, the consumer doesn't need to know that it's been constructed of multiple definitions (unlike oneOf or anyOf). Therefore, the template can just render the combined children.

When we're using 'allOf', * if all elements in the allOf allows additional properties, then this will work * other set-ups are likely edge cases, as the properties would need to overlap so they're accepted by all

jamescrowley · 2020-04-28T08:18:20Z

@derberg I've re-based. I'm leaning towards just removing the 'additionalProperties' display entirely, given it's not really clear what the author's intent would be? While technically they might be permitted (if additionalProperties is missing or set to true), but that could just be due to the json schema limitations. Thoughts?

derberg · 2020-04-29T15:47:46Z

@jamescrowley I merged into master some refactor of filters, you will have to rebase and as a result modify the filters a bit.

Also please extend asyncapi.yml with allOf use case as we always want to have a new feature represented in the example. I wrote about it here #16 (comment)

derberg · 2020-04-29T16:08:12Z

I also played with the template a bit. This is the spec I was using:

asyncapi: 2.0.0

info:
  title: Signup service example (internal)
  version: 0.1.0

channels:
  /user/signedup:
    subscribe:
      message:
        payload:
          $ref: "#/components/schemas/Cat"

components:
  schemas:
    Pet:
      type: object
      discriminator: petType
      properties:
        name:
          type: string
        petType:
          type: string
      required:
      - name
      - petType
    Cat:
      description: A representation of a cat
      allOf:
      - $ref: '#/components/schemas/Pet'
      - type: object
        properties:
          huntingSkill:
            type: string
            description: The measured skill for hunting
            enum:
            - clueless
            - lazy
            - adventurous
            - aggressive
        required:
        - huntingSkill
    Dog:
      description: A representation of a dog
      allOf:
      - $ref: '#/components/schemas/Pet'
      - type: object
        properties:
          packSize:
            type: integer
            format: int32
            description: the size of the pack the dog is from
            minimum: 0
        required:
        - packSize
    StickInsect:
      description: A representation of an Australian walking stick
      allOf:
      - $ref: '#/components/schemas/Pet'
      - type: object
        properties:
          petType:
            const: StickBug
          color:
            type: string
        required:
        - color

This is how it looks like with current template:

This is how it looks like with your PR:

You can see that below is missing in your PR, we shoudl be able to collapse/expande object payload

2. I'm also super puzzled about one thing. It is not clear for the user that the given payload is `allOF` anymore and I'm thinking if it is not bad, because the payload part should reflect the reality and the example should be flattened as it is not. Thoughts?

fmvilas · 2020-04-29T16:35:37Z

I'm also super puzzled about one thing. It is not clear for the user that the given payload is allOF anymore and I'm thinking if it is not bad, because the payload part should reflect the reality and the example should be flattened as it is not. Thoughts?

I think this is a valid concern. It could probably be configured using a template param? So people can decide what to do. For instance, for public APIs, you might not want to share your AsyncAPI files. However, if it's something internal and you let people inspect the AsyncAPI file, it's useful to have an indicator that you're using inheritance.

We can also have just an indicator nearby the schema using allOf. Just remember not only the payload may be using allOf.

jamescrowley · 2020-04-30T12:41:36Z

There are other problems here unfortunately. Within allOf, you can define the same properties in each definition, with different levels of 'specificity' on the constraint (ie, maybe just type: object, and another schema one putting tighter restrictions on it). Right now, the template just ends up listing the properties twice. In theory you could try and merge this definitions into the most constrained version, but we're definitely veering into complex territory. It may be worth consulting closer with how OpenAPI approaches this.

I may have bitten off more than I can chew trying to generalise this right now - in our internal version, I'm just taking the 'last' occurrence of the property name as the 'most constrained, but that's not exactly generally applicable!

github-actions · 2020-06-30T00:18:03Z

This pull request has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 30 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.
Thank you for your contributions ❤️

jamescrowley force-pushed the flattenAllOf2 branch from 830eb0e to 6ec4393 Compare April 26, 2020 23:24

jamescrowley commented Apr 26, 2020

View reviewed changes

jamescrowley added 2 commits April 28, 2020 18:14

Flatten the appearance of 'allOf' schema elements

f1ed4c0

In the case of 'allOf' schema elements, the consumer doesn't need to know that it's been constructed of multiple definitions (unlike oneOf or anyOf). Therefore, the template can just render the combined children.

Handle additionalProperties in 'allOf' case

a5c8d31

When we're using 'allOf', * if all elements in the allOf allows additional properties, then this will work * other set-ups are likely edge cases, as the properties would need to overlap so they're accepted by all

jamescrowley force-pushed the flattenAllOf2 branch from 6ec4393 to a5c8d31 Compare April 28, 2020 08:14

jamescrowley changed the title ~~Flatten allOf cases in message payloads~~ feat: flatten allOf cases in message payloads Apr 28, 2020

derberg mentioned this pull request May 7, 2020

Usages of allOf within message payload could be flattened asyncapi/asyncapi-react#596

Open

github-actions bot added the stale label Jun 30, 2020

github-actions bot closed this Jul 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: flatten allOf cases in message payloads #16

feat: flatten allOf cases in message payloads #16

jamescrowley commented Apr 26, 2020 •

edited

Loading

jamescrowley Apr 26, 2020

jamescrowley Apr 26, 2020

derberg Apr 27, 2020

derberg commented Apr 27, 2020

jamescrowley commented Apr 27, 2020

derberg commented Apr 28, 2020

jamescrowley commented Apr 28, 2020

derberg commented Apr 29, 2020

derberg commented Apr 29, 2020

fmvilas commented Apr 29, 2020

jamescrowley commented Apr 30, 2020 •

edited

Loading

github-actions bot commented Jun 30, 2020



		Nunjucks.addFilter('additionalPropertiesAllowedForAll', (items) => items.every(p => p.additionalProperties() === true \|\| p.additionalProperties() === undefined \|\| p.additionalProperties() === null));
		Nunjucks.addFilter('additionalPropertiesDisabledForAll', (items) => items.every(p => p.additionalProperties() === false) );

feat: flatten allOf cases in message payloads #16

feat: flatten allOf cases in message payloads #16

Conversation

jamescrowley commented Apr 26, 2020 • edited Loading

jamescrowley Apr 26, 2020

Choose a reason for hiding this comment

jamescrowley Apr 26, 2020

Choose a reason for hiding this comment

derberg Apr 27, 2020

Choose a reason for hiding this comment

derberg commented Apr 27, 2020

jamescrowley commented Apr 27, 2020

derberg commented Apr 28, 2020

jamescrowley commented Apr 28, 2020

derberg commented Apr 29, 2020

derberg commented Apr 29, 2020

fmvilas commented Apr 29, 2020

jamescrowley commented Apr 30, 2020 • edited Loading

github-actions bot commented Jun 30, 2020

jamescrowley commented Apr 26, 2020 •

edited

Loading

jamescrowley commented Apr 30, 2020 •

edited

Loading