Feature: oas validatie pre commit #75

MelvLee · 2020-09-11T05:26:51Z

Gebruik van pre-commit Git Hook om OAS file te valideren voordat wijzigingen worden gecommit.

Pre-requisite: npm. Deze kan worden geïnstalleerd door node.js te installeren.

In de haal-centraal-common map de volgende statements aanroepen:

npm install (alleen nodig bij eerste keer of als npm packages moeten worden ge-update)
npm run lint (Spectral wordt gebruikt om common.yaml te valideren)

Bij elke commit wordt nu de npm run lint statement automatisch aangeroepen. Bij errors wordt de commit afgebroken.

In deze p.r. zitten alleen nog warnings. Om te testen dat bij een commit de common.yaml wordt gevalideerd, kan je de path: {} regel uit common.yaml verwijderen en de wijziging committen.

Ik heb ervoor gekozen om Spectral als OAS linter te gebruiken omdat het mogelijk is om eigen validatie regels toe te voegen. De validatie regels worden geschreven in yaml. Ook is het makkelijk om validatie regels uit te zetten.

Voorbeeld van een validatie regel:

  schema-names-pascal-case:
    description: Schema names MUST be written in PascalCase
    message: '{{property}} is not PascalCase: {{error}}'
    recommended: true
    type: style
    given: '$.components.schemas.*~'
    then:
      function: pattern
      functionOptions:
        match: '^[A-Z][a-zA-Z0-9]*$'

.spectral.yml is een custom ruleset die de Spectral ruleset extend. Ik heb validaties toegevoegd voor Camelcase, PascalCase en inline object als property

- aanroep lint bij pre-commit - toevoegen van paths: {} is common.yaml tbv lint

fsamwel · 2020-09-11T12:13:29Z

.spectral.yml

+    message: '{{path}} is gedefinieerd als een inline object. Definieer deze als een schema component en verwijs hiernaar met $ref'
+    type: style
+    resolved: false
+    given: '$.components.schemas..properties[?(@.type === "object")]'


dit werkt alleen wanneer ook echt type: "object is opgenomen. Dit is default dus kan je ook weglaten.

Als ik dus in common.yaml type: object weghaal wordt geen Warning meer gegeven, bijvoorbeeld bij property first:

HalPaginationLinks: allOf: - $ref: "#/components/schemas/HalCollectionLinks" - type: "object" properties: first: description: "uri voor het opvragen van de eerste pagina van deze collectie" properties: href: type: "string" example: "/resourcenaam?page=1" templated: type: boolean title: type: "string" example: "Eerste pagina"

Kan je deze niet beter definiëren met:

given: '$.components.schemas..properties[*].properties' then: function: falsy

fsamwel · 2020-09-11T12:40:56Z

.spectral.yml

+    type: style
+    given: '$.components.schemas.*~'
+    then:
+      function: pattern


Waarom gebruik je hier niet:

then: function: casing functionOptions: type: pascal

fsamwel · 2020-09-11T12:42:52Z

.spectral.yml

+    given: '$.components.parameters[?(@.in == "query")]'
+    then:
+      field: name
+      function: pattern


Waarom gebruik je hier niet:

then:
function: casing
functionOptions:
type: camel

fsamwel · 2020-09-11T13:13:50Z

Misschien kan je deze ook toevoegen:

  enum-value-snake-case:
    description: Enumeration values SHOULD be in snake case
    message: 'Enumeratiewaarde "{{value}}" is niet snake_case: {{path}}'
    recommended: true
    resolved: false
    type: style
    given: '$.components.schemas..enum[*]'
    then:
      function: casing
      functionOptions:
        type: snake

fsamwel · 2020-09-11T13:32:56Z

En deze ook:

  allof-extends-object:
    description: allOf must extend an object and add at least one property
    message: '{{path}} moet beginnen met $ref'
    resolved: false
    type: style
    given: '$.components.schemas[*].allOf[0]'
    then:
      - field: "$ref"
        function: truthy
  allof-adds-property:
    description: allOf must extend an object and add at least one property
    message: '{{path}}: het tweede item in een allOf moet ten minste één property toevoegen'
    resolved: false
    type: style
    given: '$.components.schemas[*].allOf[1]'
    then:
      - field: "properties"
        function: truthy
  allof-single-extention:
    description: allOf must extend from exactly one object
    message: '{{path}}: allOf mag niet meer dan twee items bevatten, de eerste met $ref, de tweede met properties'
    resolved: false
    type: style
    given: '$.components.schemas[*].allOf[2]'
    then:
      function: falsy

MelvLee · 2020-09-14T06:23:53Z

Kan je deze niet beter definiëren met:

    given: '$.components.schemas..properties[*].properties'
    then:
      function: falsy

👍, ga ik aanpassen

MelvLee · 2020-09-14T06:33:54Z

Waarom gebruik je hier niet:

then:
function: casing
functionOptions:
type: camel

Nice. Mijn oplossing was gewoon een copy paste uit https://github.com/openapi-contrib/style-guides/blob/master/openapi.yml. Ik had niet gezien dat deze als Core Function beschikbaar is. Zal ik aanpassen

- toevoegen van openapi contact en license info

MelvLee · 2020-09-14T06:48:19Z

Misschien kan je deze ook toevoegen:

  enum-value-snake-case:
    description: Enumeration values SHOULD be in snake case
    message: 'Enumeratiewaarde "{{value}}" is niet snake_case: {{path}}'
    recommended: true
    resolved: false
    type: style
    given: '$.components.schemas..enum[*]'
    then:
      function: casing
      functionOptions:
        type: snake

👍

MelvLee · 2020-09-14T06:48:46Z

En deze ook:

  allof-extends-object:
    description: allOf must extend an object and add at least one property
    message: '{{path}} moet beginnen met $ref'
    resolved: false
    type: style
    given: '$.components.schemas[*].allOf[0]'
    then:
      - field: "$ref"
        function: truthy
  allof-adds-property:
    description: allOf must extend an object and add at least one property
    message: '{{path}}: het tweede item in een allOf moet ten minste één property toevoegen'
    resolved: false
    type: style
    given: '$.components.schemas[*].allOf[1]'
    then:
      - field: "properties"
        function: truthy
  allof-single-extention:
    description: allOf must extend from exactly one object
    message: '{{path}}: allOf mag niet meer dan twee items bevatten, de eerste met $ref, de tweede met properties'
    resolved: false
    type: style
    given: '$.components.schemas[*].allOf[2]'
    then:
      function: falsy

👍

fsamwel · 2020-09-14T08:13:08Z

@JohanBoer Bij een fout (bijv. weghalen "path: {}") zie ik (in Atom) dat de commit mislukt en krijg ik alle fouten én warnings te zien. Maar bij alleen warnings (iets anders wijzigen zonder fout) gaat de commit door en zie ik de warnings niet. Wanneer ik de commit doe in de CLI zie ik die wel. Hoe doe jij je commits? Kan jij ook testen of jij in jouw werkwijze waarschuwingen te zien krijgt?

fsamwel · 2020-09-14T08:35:40Z

.spectral.yml

+    then:
+      - field: "properties"
+        function: truthy
+  allof-single-extention:


mijn schuld (je hebt dit van mij overgenomen). Typefout extention --> extension

fsamwel · 2020-09-14T09:13:41Z

.spectral.yml

+    then:
+      function: casing
+      functionOptions:
+        type: pascal


Gevolg is dat alle *_links, *_embedded, *_enum en *_tabel componenten een waarschuwing geven. Het opnemen van een underscore is namelijk niet in lijn met UpperCamelCase/PascalCase.

Vanuit consistentie zou ik het ook beter vinden deze underscore weg te halen.

Kan dat nu nog zonder grote consequenties? In BAG en BRK hebben we ook underscore in de schemacomponent namen gebruikt (dan ontstaat er weer inconsistentie tussen de Haal Centraal API's). Of kunnen we die ook aanpassen in de eerstvolgende release? M.a.w. levert dat breaking changes op bij iemand?

Issue #75 hiervoor gemaakt

Bij *_links, *_embedded, *_tabel leidt het tot een breaking change bij de providers, niet bij de consumers omdat de consumers niets doen met de classes zelf. Voorwaarde is alleen hernoemen van de schema naam en niet property namen.
Voor de _enums zal ik het testen

_links en _embedded zijn reserved words in hal+json. Als je de underscores weghaalt dan wordt het plain json en moet je het content-type erop aanpassen. Dit is een afweging. Vind je consistentie in de naamgeving van properties belangrijker dan het voldoen aan de draft van json hal. Mijn voorkeur gaat naar het handhaven van de underscores bij _links en _embedded.

_links en _embedded zijn reserved words in hal+json. Als je de underscores weghaalt dan wordt het plain json en moet je het content-type erop aanpassen. Dit is een afweging. Vind je consistentie in de naamgeving van properties belangrijker dan het voldoen aan de draft van json hal. Mijn voorkeur gaat naar het handhaven van de underscores bij _links en _embedded.

Even een check. @fsamwel, je hebt hier alleen over de component namen toch? Niet over de properties, want hernoemen van propertynamen leidt tot breaking changes bij zowel consumers als providers.
Ik ben het dus eens met @JohanBoer om _embedded en _links property namen niet te veranderen en ook hal json te blijven als content type.
Ik ga kijken of de jsonpath kan worden aangepast zodat de _links en _embedded properties niet worden meegenomen in de casing check

ja, het gaat hier om de componentnamen, niet om de propertynamen.

Ik denk dat het in de validatie kan worden aangepast door toch niet de built in functie casino (Pascal) te gebruiken, maar zelf een reguliere expressie te maken die deze extensies wel toelaat:
/^([A-Z][a-z0-9]+)+(_embedded|_links|_enum|_tabel)?$/

JohanBoer · 2020-09-14T09:15:06Z

@JohanBoer Bij een fout (bijv. weghalen "path: {}") zie ik (in Atom) dat de commit mislukt en krijg ik alle fouten én warnings te zien. Maar bij alleen warnings (iets anders wijzigen zonder fout) gaat de commit door en zie ik de warnings niet. Wanneer ik de commit doe in de CLI zie ik die wel. Hoe doe jij je commits? Kan jij ook testen of jij in jouw werkwijze waarschuwingen te zien krijgt?

Ik krijg geen warning te zien in Atom. Blijkbaar constateert die linter de warnings niet. Ik doe mijn commits in Atom via het Git tabje.

MelvLee · 2020-09-14T09:16:26Z

@JohanBoer Bij een fout (bijv. weghalen "path: {}") zie ik (in Atom) dat de commit mislukt en krijg ik alle fouten én warnings te zien. Maar bij alleen warnings (iets anders wijzigen zonder fout) gaat de commit door en zie ik de warnings niet. Wanneer ik de commit doe in de CLI zie ik die wel. Hoe doe jij je commits? Kan jij ook testen of jij in jouw werkwijze waarschuwingen te zien krijgt?

In Visual Studio Code heb ik een output window waar ik de git logs kan zien. Ik moet dan wel de in de output dropdown git selecteren. Misschien heeft Atom ook iets soortgelijks

MelvLee · 2020-09-14T09:21:49Z

Ik krijg geen warning te zien in Atom. Blijkbaar constateert die linter de warnings niet. Ik doe mijn commits in Atom via het Git tabje.

Als we tijdens commit warning als error willen zien, dan moet je -F warn toevoegen

fsamwel · 2020-09-14T13:07:45Z

@MelvLee zou je de allof regels nog eens kunnen aanpassen. het blijkt dat allOf op verschillende plekken kan voorkomen. Dus moet de given ook dieper zoeken

allof-extends-object:
   description: allOf must extend an object and add at least one property
   message: '{{path}} moet beginnen met $ref'
   resolved: false
   type: style
   given: '$..allOf[0]'
   then:
     - field: "$ref"
       function: truthy
 allof-adds-property:
   description: allOf must extend an object and add at least one property
   message: '{{path}}: het tweede item in een allOf moet ten minste één property toevoegen'
   resolved: false
   type: style
   given: '$..allOf[1]'
   then:
     - field: "properties"
       function: truthy
 allof-single-extension:
   description: allOf must extend from exactly one object
   message: '{{path}}: allOf mag niet meer dan twee items bevatten, de eerste met $ref, de tweede met properties'
   resolved: false
   type: style
   given: '$..allOf[2]'
   then:
     function: falsy

Bijvoorbeeld deze werd niet gevonden:

Verblijfplaatshistorie:
      allOf:
      - $ref: "#/components/schemas/Verblijfplaats"
      - properties:
          datumTot:
            allOf:
              - $ref: "https://raw.githubusercontent.com/VNG-Realisatie/Haal-Centraal-common/v1.1.0/api-specificatie/common.yaml#/components/schemas/Datum_onvolledig"
              - description: "De datum vanaf wanneer de persoon niet meer op het adres verblijft."

MelvLee · 2020-09-14T13:30:39Z

@MelvLee zou je de allof regels nog eens kunnen aanpassen. het blijkt dat allOf op verschillende plekken kan voorkomen. Dus moet de given ook dieper zoeken

allof-extends-object:
   description: allOf must extend an object and add at least one property
   message: '{{path}} moet beginnen met $ref'
   resolved: false
   type: style
   given: '$..allOf[0]'
   then:
     - field: "$ref"
       function: truthy
 allof-adds-property:
   description: allOf must extend an object and add at least one property
   message: '{{path}}: het tweede item in een allOf moet ten minste één property toevoegen'
   resolved: false
   type: style
   given: '$..allOf[1]'
   then:
     - field: "properties"
       function: truthy
 allof-single-extension:
   description: allOf must extend from exactly one object
   message: '{{path}}: allOf mag niet meer dan twee items bevatten, de eerste met $ref, de tweede met properties'
   resolved: false
   type: style
   given: '$..allOf[2]'
   then:
     function: falsy

Bijvoorbeeld deze werd niet gevonden:

Verblijfplaatshistorie:
      allOf:
      - $ref: "#/components/schemas/Verblijfplaats"
      - properties:
          datumTot:
            allOf:
              - $ref: "https://raw.githubusercontent.com/VNG-Realisatie/Haal-Centraal-common/v1.1.0/api-specificatie/common.yaml#/components/schemas/Datum_onvolledig"
              - description: "De datum vanaf wanneer de persoon niet meer op het adres verblijft."

👍

fsamwel · 2020-09-15T08:21:31Z

Ook denk ik een leuke regel:, om te checken dat we niet onnodig gedetailleerd responses definiëren:

  too-much-information:
    description: properties of GET responses should not use pattern, minimum, maximum, minLength, maxLength, minItems or required
    resolved: false
    type: style
    message: 'Gebruik geen {{property}} in response definitie: {{path}}'
    given: '$.components.schemas..properties[*]'
    then:
      - field: "minItems"
        function: falsy

      - field: "maxItems"
        function: falsy

      - field: "maxLength"
        function: falsy

      - field: "maxLength"
        function: falsy

      - field: "minimum"
        function: falsy

      - field: "maximum"
        function: falsy

      - field: "required"
        function: falsy

      - field: "pattern"
        function: falsy

- too-much-information rule toegevoegd - lint script naam hernoemd naar oas:lint t.b.v. groeperen

MelvLee added 2 commits September 11, 2020 06:55

feature: pre-commit oas validatie

56428a1

fix:

9054e73

- aanroep lint bij pre-commit - toevoegen van paths: {} is common.yaml tbv lint

MelvLee requested review from JohanBoer, melsk-r and fsamwel September 11, 2020 05:26

feature: custom spectral ruleset

59c43df

MelvLee marked this pull request as ready for review September 11, 2020 05:39

fsamwel reviewed Sep 11, 2020

View reviewed changes

fsamwel requested changes Sep 11, 2020

View reviewed changes

fsamwel reviewed Sep 11, 2020

View reviewed changes

- Uitbreiden custom spectral rules

8855b02

- toevoegen van openapi contact en license info

fsamwel reviewed Sep 14, 2020

View reviewed changes

fsamwel mentioned this pull request Sep 14, 2020

verwijderen vaste naamgeving componenten met underscore #76

Open

fix: typo in name

63ae820

fix: allOF op elk niveau valideren

e27f6fd

fsamwel mentioned this pull request Sep 15, 2020

Linter validaties uitbreiden op basis van ontwerprichtlijnen #78

Open

lint:

603da70

- too-much-information rule toegevoegd - lint script naam hernoemd naar oas:lint t.b.v. groeperen

MelvLee merged commit fe61adb into master Sep 16, 2020

MelvLee deleted the feature/oas-validatie-pre-commit branch September 16, 2020 06:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: oas validatie pre commit #75

Feature: oas validatie pre commit #75

MelvLee commented Sep 11, 2020 •

edited

Loading

fsamwel Sep 11, 2020

fsamwel Sep 11, 2020

fsamwel Sep 11, 2020 •

edited

Loading

fsamwel Sep 11, 2020

fsamwel commented Sep 11, 2020

fsamwel commented Sep 11, 2020

MelvLee commented Sep 14, 2020

MelvLee commented Sep 14, 2020

MelvLee commented Sep 14, 2020

MelvLee commented Sep 14, 2020

fsamwel commented Sep 14, 2020

fsamwel Sep 14, 2020

MelvLee Sep 14, 2020

fsamwel Sep 14, 2020 •

edited

Loading

MelvLee Sep 14, 2020 •

edited

Loading

JohanBoer Sep 14, 2020

MelvLee Sep 14, 2020

fsamwel Sep 14, 2020

JohanBoer commented Sep 14, 2020

MelvLee commented Sep 14, 2020

MelvLee commented Sep 14, 2020

fsamwel commented Sep 14, 2020

MelvLee commented Sep 14, 2020

fsamwel commented Sep 15, 2020

Feature: oas validatie pre commit #75

Feature: oas validatie pre commit #75

Conversation

MelvLee commented Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fsamwel Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fsamwel commented Sep 11, 2020

fsamwel commented Sep 11, 2020

MelvLee commented Sep 14, 2020

MelvLee commented Sep 14, 2020

MelvLee commented Sep 14, 2020

MelvLee commented Sep 14, 2020

fsamwel commented Sep 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fsamwel Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

MelvLee Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanBoer commented Sep 14, 2020

MelvLee commented Sep 14, 2020

MelvLee commented Sep 14, 2020

fsamwel commented Sep 14, 2020

MelvLee commented Sep 14, 2020

fsamwel commented Sep 15, 2020

MelvLee commented Sep 11, 2020 •

edited

Loading

fsamwel Sep 11, 2020 •

edited

Loading

fsamwel Sep 14, 2020 •

edited

Loading

MelvLee Sep 14, 2020 •

edited

Loading