Geonovum / KP-APIs

26 stars 40 forks source link

Betekenis en gebruik van required properties in PATCH methoden #323

Open joeribekker opened 5 years ago

joeribekker commented 5 years ago

In de API strategie, maar ook op internet, kon ik niets vinden over het gebruik van required properties bij GET en PATCH methoden, en ik vind de interpretatie van de definitie in JSON Schema Validation geen uitkomst bieden:

6.5.3. required The value of this keyword MUST be an array. Elements of this array, if any, MUST be strings, and MUST be unique. An object instance is valid against this keyword if every item in the array is the name of a property in the instance. Omitting this keyword has the same behavior as an empty array.

Dat een property required is betekent dus dat het niet persé een waarde moet hebben (veel voorkomende misvatting) maar dat het property in het object moet zitten. Zie ook voorbeelden van objects zonder en met required

Neem onderstaande OAS waar een object Quote het required attribuut tekst heeft.

  /quotes/{uuid}:
    get:
      operationId: quote_read
      description: Bekijk een enkele quote.
      responses:
        '200':
          description: ''
          content:
            application/json:
              schema:
                required:
                - tekst
                type: object
                properties:
                  tekst:
                    title: Quote
                    type: string
                  bronNaam:
                    title: Bron naam
                    type: string

Aantal vragen hierover:

  1. Bij een GET krijg ik dus zeker weten het property tekst terug krijg maar het is niet zeker dat ik bronNaam terug krijg (bijvoorbeeld door compactere versie van een object in lists, of door autorisaties)?
  2. Mag je bij een PATCH tekst toch weglaten omdat PATCH sowieso al impliceert dat ik slechts enkele properties wil updaten?
  3. Mag je bij een POST bronNaam weglaten, ook al wordt daar (typisch) verwacht dat je de volledige nieuw te maken state meegeeft?
  4. Zouden de required properties verschillen tussen al deze methoden, wat impliceert dat we niet makkelijk $ref: '#/components/schemas/Quote' zouden kunnen gebruiken waar een eenduidige definitie staat van het object zodat we het niet bij elke methode inline hoeven te specificeren?
fsamwel commented 5 years ago

ik heb even gecheckt wat code generatoren doen. Heb er twee gecheckt (csharp en python clients voor ZRC in editor.swagger.io) en beide geven een foutmelding wanneer een required parameter niet aanwezig is of null/None.

Naar mijn idee:

En dan concludeer ik: Ad 1 moet tekst in het antwoord zitten, mag die een lege string zijn (want er is geen minLength gedefinieerd), mag die niet null zijn Ad 1 mag bronNaam worden weggelaten in het antwoord, mag deze null zijn of een string waarde (incl. lege string) Ad 2 mag je bij een PATCH geen property als required definiëren Ad 3 geeft gegenereerde code een foutmelding wanneer je bij een post required veld tekst weglaat Ad 4 ontkom je er dan niet aan verschillende definities te maken voor PATCH en POST

joeribekker commented 5 years ago

Ik begrijp niet helemaal wat je gecontroleerd hebt met code-generators. Je hebt code gegenereerd van een OAS en daar iets mee gedaan, maar ik begrijp niet hoe dat precies mijn vragen beantwoord en tot jouw antwoorden leidt. Code generators doen gewoon wat er in de OAS staat? Voor de volledigheid ga ik wel in op je conclusies.

Naar mijn idee: betekent required hetzelfde voor een request parameter (query of path) of een property in een request/response body en ongeacht de operatie

Ja, volgens mij ook maar ik zou dat graag wat sterker terug zien dan "naar mijn idee", bijvoorbeeld door officiele voorbeelden of in RFCs. Als je naar punt 2 gaat hieronder zie je meteen dat OAS validators er in ieder geval niet zo over denken.

Ad 1 moet tekst in het antwoord zitten, mag die een lege string zijn (want er is geen minLength gedefinieerd), mag die niet null zijn Ad 1 mag bronNaam worden weggelaten in het antwoord, mag deze null zijn of een string waarde (incl. lege string)

Check, volgens mij ook.

Ad 2 mag je bij een PATCH geen property als required definiëren

Dat zegt mijn gevoel ook echter zijn OAS-validators het daar niet mee eens. Zie bijvoorbeeld deze die Petstore als sample gebruikt. Als je daar zelf nog even PATCH aan toevoegt valideert het prima ondanks dat Pet (als referentie) name als required heeft (DIY hieronder).

  "/pets/{name}":
    put:
      description: Update pet
      parameters:
        - name: name
          in: path
          description: Name of the pet to update
          required: true
          type: string
      responses:
        "200":
          description: pet response
          schema:
            $ref: "#/definitions/pet"
        default:
          description: unexpected error
          schema:
            $ref: "#/definitions/errorModel"
  1. Mag je bij een POST tekst weglaten, ook al wordt daar (typisch) verwacht dat je de volledige nieuw te maken state meegeeft?

    Ad 3 geeft gegenereerde code een foutmelding wanneer je bij een post required veld tekst weglaat

Foutje in mijn post. Hier had bronNaam moeten staan. Ik heb dit aangepast in mijn issue voor de leesbaarheid maar daardoor is je reactie niet meer relevant. Wellicht heb je nog een inzicht hierover?

Ad 4 ontkom je er dan niet aan verschillende definities te maken voor PATCH en POST

Volgens OAS-validators (zie mijn opmerking bij 2) hoeft dat dus niet, wat het leven wel zo prettig maakt.

fsamwel commented 5 years ago

Joeri, ik denk dat je gelijk hebt dat Swagger validators het niet afkeuren wanneer je een PATCH request body definieert met required parameters. Met "mag je bij een PATCH geen property als required definiëren" bedoel ik dat een provider een request body zonder de required property gaat afkeuren. Het definiëren van een PATCH request body met required properties leidt er dus toe dat die properties inderdaad required zijn, en dat is in strijd met het idee achter PATCH.

Ik begrijp niet helemaal wat je gecontroleerd hebt met code-generators. Ik heb van de ZRC van Gemma Zaken Python (en .Net) code gegenereerd met editor.swagger.io voor een client en (i.g.v. .Net) een provider/server. Hierin heb ik gekeken of in de code iets is gedaan met de required parameters in de request body.

In de OAS is voor PATCH /zaken/{uuid} de response gedefinieerd in component Zaak met bijvoorbeeld required property "bronorganisatie". In de gegenereerde model zaak.py staat:

if bronorganisatie is None:
            raise ValueError("Invalid value for `bronorganisatie`, must not be `None`")

In de .Net code voor de provider staat in model Zaak.cs:

[Required]
[DataMember(Name="bronorganisatie")]
public string Bronorganisatie { get; set; }
frankvanes commented 3 years ago

Voor zover ik weet zijn er niet heel sterke semantics rondom PATCH beschreven, dus is de exacte uitwerking voornamelijk aan de implementatie (of specifieke standaard, als hier het geval is).

Ik vraag mij af of het niet te sterk is om te formuleren dat required velden niet moeten worden gebruikt bij PATCH operaties, omdat ik me best situaties kan voorstellen waar het gebruik van verplichte velden nuttig kan zijn. Een voorbeeld is bijvoorbeeld wanneer het hierboven beschreven Quote object binnen een ander object van een JSON object valt:

Wanneer je tekst verplicht stelt maar quote niet, kan je aangeven dat in een PATCH document de quote achterwege gelaten kan worden als deze niet gewijzigd moet worden, maar als je een quote aanpast, moet deze een tekst krijgen.

Een RFC die wellicht nuttig kan zijn bij het beschrijven van de PATCH operatie is RFC7396 (JSON Merge Patch), welke grofweg aangeeft dat een veld dat niet wordt meegegeven in het PATCH document niet moet worden aangepast, terwijl een waarde null betekent dat het veld moet worden verwijderd uit de resource.

HenriKorver commented 3 years ago

Eens, bij een PATCH-operatie moet het zeker mogelijk zijn om bepaalde velden of groepen van velden verplicht te maken zoals het voorbeeld van @frankvanes laat zien. De PATCH-standaard (https://tools.ietf.org/html/rfc5789) spreekt dit ook op geen enkele manier tegen. Het enige wat raar zou zijn als je in een PATCH alle velden verplicht zou maken, want dan kun je beter PUT gebruiken :-).

Wat wel een interessante vraag is welke rol de required property precies speelt bij het verplicht maken van velden in verschillende situaties. Bij GET en POST heeft required een duidelijke betekenis.

Bij PATCH en PUT zou je de bovenstaande semantiek op de volgende manier willen overrulen:

Met deze afspraken voor het overrulen kun je nu bij zowel GET, POST, PUT en PATCH hetzelfde Schema Object gebruiken voor de request- en responsberichten. Met als (grote) voordeel dat als je één resource wijzigt dat je niet voor elke operatie het bijbehorende schema object van de input en output datatypes hoeft aan te passen.

HenriKorver commented 3 years ago

Een RFC die wellicht nuttig kan zijn bij het beschrijven van de PATCH operatie is RFC7396 (JSON Merge Patch), welke grofweg aangeeft dat een veld dat niet wordt meegegeven in het PATCH document niet moet worden aangepast, terwijl een waarde null betekent dat het veld moet worden verwijderd uit de resource.

Deze RFC geeft mijns inziens weinig soelaas in deze discussie. RFC7396 gaat over het dynamisch kunnen toevoegen en verwijderen van resource properties. Vergelijkbaar met de ALTER TABLE statement van SQL:

ALTER TABLE Customers
ADD Email varchar(255);

Dit staat haaks op het gebruik van OAS specificaties waar de structuur van resources van te voren statisch is vastgelegd en niet via API calls kan worden veranderd.

frankvanes commented 3 years ago

Dank voor je comment Henri. Waar lees je precies dat deze RFC over het dynamisch toevoegen en verwijderen van resource properties gaat? Het klopt dat je waarden uit de content van een specifieke resource instantie kan halen of toe kunt voegen, maar wel binnen wat er binnen de OAS specificatie staat gedefinieerd: het gaat om het toevoegen en verwijderen van optionele velden die wel in de OAS specificatie staan.

Dus wat dat betreft niet vergelijkbaar met de ALTER TABLE statement van SQL, omdat de resource specificatie niet wordt aangepast. Eerder het toevoegen van een NULL waarde in een veld dat NULL mag zijn.

MelvLee commented 3 years ago

Ik heb het gevoel dat in deze discussie een aantal concepten niet correct worden geïnterpreteerd.

  1. een Resource komt overeen met een entiteit of database tabel. In dit voorbeeld is Quote waarschijnlijk een entiteit of een tabel en kan dus worden gemoduleerd als een Resource, maar verhuizen, huwelijk of splitsen van een kadastraal onroerende zaak kan ook als een Resource worden gemoduleerd. Zoals Roy Fielding het in zijn proefschrift aangeeft: Any information that can be named can be a resource: a document or image, a temporal service (e.g. "today's weather in Los Angeles"), a collection of other resources, a non-virtual object (e.g. a person), and so on.
  2. De HTTP verbs POST / GET / PUT en PATCH / DELETE is gelijk aan CREATE / SELECT / UPDATE / DELETE. Dit kan, maar is dat altijd handig? Stel ik wil de Quote kunnen publiceren, dan kan ik dit mogelijk maken door de PATCH verb te gebruiken en de status kenmerk te wijzigen naar gepubliceerd. Maar wat als een consumer ook andere kenmerken meestuurt? Wat betekent de PATCH dan? Kan je als je de tekst van een Quote aanpast ook tegelijk de status wijzigen? En als ik het publiceren van de Quote ongedaan wil maken, hoe doe ik dat dan? Met de DELETE? Of met de PATCH?
  3. Eén definitie voor een Resource. Misschien is dit een wens, maar persoonlijk denk ik dat het niet mogelijk is. In de Quote is bijvoorbeeld de identifier weggelaten. En die heb ik minimaal nodig om de PUT, PATCH en DELETE te kunnen gebruiken en is deze dus verplicht. Maar wil ik de Resource definitie met identifier ook gebruiken voor POST, PUT en PATCH? Wil ik dat consumers de identifier van een Resource kunnen wijzigen? Misschien is dit gewenste functionaliteit, maar als dat niet het geval is dan heb je minimaal 2 definities van een Resource. Je kan de allOf constructie gebruiken om zoveel mogelijk herhaling te voorkomen, maar je ontkomt niet aan meerdere definities voor één Resource.

Naar mijn mening zou een API in gebruik simpel moeten zijn. Eén PATCH methode in combinatie met het overrulen van verplichte kenmerken om zowel 'tekst wijzigen', 'publiceren' en 'publiceren ongedaan maken' mogelijk te maken, maakt de API niet simpel. En wat als er bij het ongedaan maken ook de reden moet worden opgeven? Dit wil je niet toevoegen aan de Quote definitie.

HenriKorver commented 3 years ago

Dank voor je comment Henri. Waar lees je precies dat deze RFC over het dynamisch toevoegen en verwijderen van resource properties gaat?

In het voorbeeld (https://tools.ietf.org/html/rfc7396#section-3) wordt run-time een nieuwe property /member aan het object toegevoegd, namelijk "phoneNumber"

A user agent wishing to change the value of the "title" member from "Goodbye!" to the value "Hello!", add a new "phoneNumber" member, remove the "familyName" member from the "author" object, and replace the "tags" array so that it doesn't include the word "sample" would send the following request:

Je hebt gelijk dat als je alleen maar velden toevoegt of verwijdert die van te voren als optionele elementen in de OAS gedefinieerd zijn, dat je dan geen functionaliteit zoals "ALTER TABLE" nodig hebt. Echter dit wordt niet verondersteld in deze RFC. Deze RFC kent überhaupt niet de notie van een achterliggend JSON- of OAS-schema, vandaar wellicht de verwarring.

Hoe dan ook interpreteer ik add a new "phoneNumber" member als het toevoegen van een nieuwe property die er eerder niet was.

Zelfde verhaal voor remove the "familyName" member from the "author" object. Dat interpreteer ik als het (definitief) verwijderen van een property die niet meer nodig is en niet als het tijdelijk even niet gebruiken van een optioneel veld.

Vooralsnog voor mij dus een verwarrende RFC die weinig hulp biedt in deze discussie.

joostfarla commented 3 years ago

Ik denk dat je het antwoord vooral moet zoeken in de media type specificaties (RFCs). Het media type beschrijft namelijk hoe request- of response payloads zouden moeten worden gestructureerd. Het JSON schema, als onderdeel van de OAS specificatie, zou daar vervolgens mee overeen moeten komen. Een PATCH method kent 2 gangbare media types:

JSON Merge Patch (RFC7396)

PATCH /books/1
Content-Type: application/merge-patch+json

{
    "title": "New title",
    "subsitle" null
}

Bij dit media type geef je alleen de velden mee die je wil modificeren. Wil je een property verwijderen (unsetten), dan geef je waarde null mee. Deze kenmerken maakt dit media type idempotent (meerdere malen hetzelfde request verwerken resulteert in een gelijke toestand van de resource).

JSON Patch (RFC6902)

PATCH /books/1
Content-Type: application/json-patch+json

[
    { "op": "replace", "path": "/title", "value": "New title" },
    { "op": "remove", "path": "/subtitle"}
]

Bij dit media type kun je echt muteren, door middel van een lijst van operaties (add, replace, remove etc), die in die volgorde door de server zouden moeten worden "afgespeeld" op de toestand van de resource op dat moment. Dit media type is wat complexer in gebruik, maar biedt aan de andere kant wel meer flexibiliteit; zo zou je bijvoorbeeld 1 element kunnen toevoegen aan een bestaande array. Dat maakt dit media type, in tegenstelling tot JSON Merge Patch niet idempotent (2 maal een "add" operatie op een array resulteert in 2 nieuwe elementen ipv 1).

Zou je deze request payloads willen beschrijven in een OAS spec (dmv JSON schema), dan resulteert dit dus ook in 2 verschillende schema definities.

JSON Merge Patch (RFC7396)

application/merge-patch+json:
  type: object
  properties:
    title: 
      type: string
    subtitle:
      type: string
      nullable: true

Dit betekent overigens (antwoord op de initiële vraag) dat er nooit required properties zouden mogen bestaan, m.u.v. objecten als array items. Wel zouden bepaalde properties nullable kunnen zijn, als ze ook verwijderbaar zouden moeten zijn. Om toch hergebruik te kunnen maken van een bestaande schema component, zou je een ref op kunnen nemen die enkel naar de properties wijst. Bijv:

application/merge-patch+json:
  type: object
  properties:
    $ref: '#/components/schemas/Book/properties'

JSON Patch (RFC6902)

application/json-patch+json:
  type: array
  items:
    type: object
    required:
      - op
      - path
    properties:
      op:
        type: string
        enum:
          - add
          - replace
          - remove
          - ...
      path:
        type: string 
      value:
        oneOf:        
          - type: string
          - type: number
          - type: boolean
          - type: array

Hiervoor zou je dus een generieke schema component (bijv PatchOperationList) kunnen (her)gebruiken. De oneOfconstructie zou vanaf OAS v3.1 overigens simpeler kunnen, omdat de waarde van type dan een array mag zijn (in lijn met JSON schema spec).

MelvLee commented 3 years ago

Ik denk dat deze twee media type specificaties alleen bruikbaar zijn als je properties van resources wil wijzigen die weinig tot geen business logica bevatten. Zodra je maar een beetje business logica heb, dan is dit naar mijn mening niet de goede manier om wijzigingen door te voeren. Kan je bijv. de Quote wijzigen als hij al is gepubliceerd of moet je de publicatie eerst ongedaan maken. Als dit kan, dan moet de provider naar de consumers communiceren dat ze eerst de status moeten wijzigen van gepubliceerd naar een status waarbij je wel mag wijzigen en dan pas de tekst wijzigen. Dit betekent het verplaatsen van implementatie van business logica van provider naar consumers en leidt automatisch tot tight coupling. Want op het moment dat de provider de business logica wijzigt, moeten alle consumers hun implementatie aanpassen om aan de business logica te voldoen.

joostfarla commented 3 years ago

Een JSON schema dwingt alleen maar een bepaalde documentstructuur af, zonder naar de inhoud te kijken. Een server mag altijd additionele business rules valideren om op basis van inhoud bepaalde mutaties te verbieden (bijv om geen wijzigingen toe te staan als de resource een status "gepubliceerd" heeft). Verder heb je ook, als daar een goed reden voor is, altijd de mogelijkheid om bepaalde business logic te modelleren als afzonderlijke subresources (zie API-10, puntje 2).

HenriKorver commented 3 years ago

Ik denk dat je het antwoord vooral moet zoeken in de media type specificaties (RFCs).

Daar zit wat mij betreft het probleem niet. Hoe ik mijn PATCH operaties ontwerp dat lijkt heel erg op RFC7396, hoewel deze RFC wel heel verwarrend geformuleerd is (zoals ik al eerder beschreef). Maar met de juiste interpretatie zouden we er uiteindelijk misschien wel mee uit de voeten kunnen, op voorwaarde dat dit patroon ook wordt toegestaan (zie Appendix A ):

Original Patch Result
{"a":"b", "b":null} {"b":"c"} {"a":"b", "b":"c"}

Ik hoop dat dit een onbewuste omissie is. In onze API's worden properties met null waarden vaak expliciet in de JSON documenten opgenomen. Bijvoorbeeld in een GET wil je (indien je de juiste rechten hebt) zoveel mogelijk properties van de resource geretourneerd zien, ook die geen waarde hebben (waarde null). Je ziet meteen alle properties van het object die relevant voor je kunnen zijn ook al hebben sommigen nu nog een lege waarde.

Maar het gaat mij vooral om de rol van het required statement van OAS. Het is vervelend als je voor elke HTTP operatie een apart schema moet definiëren. Ik wil pleiten voor de volgende design rule.

API-XX: In case of the PATCH operation the `required` property of OAS may be overruled.

Dus API designers die toch heel graag een apart schema definiëren voor elke HTTP-operatie kunnen gerust hun gang gaan en een complete freakshow maken van de complexiteit van hun OAS-schema. Maar designers die dat lelijk en onhandig vinden en de dingen simpel willen houden kunnen met behulp van deze design rule gewoon hetzelfde schema hergebruiken voor alle operaties. Kleine design rule, kleine moeite, maar groot plezier.

joostfarla commented 3 years ago

Dit is een vaak voorkomende verwarring over het gebruik van het required attribuut. Dit staat eigenlijk los van OAS, maar is onderdeel van de JSON schema specificatie, waar OAS sterk op leunt. De required property heeft betrekking op de aanwezigheid van bepaalde keys binnen een object. Dat staat volledig los van de waarde, en of die null mag zijn of niet. Voor het toestaan van null waardes voorziet OAS in een nullable property, iets wat overigens in de nieuwe versie (v3.1) gaat veranderen.

Nullable property in OAS v3.0:

type: object
properties:
  subtitle:
    type: string
    nullable: true

Nullable property in OAS v3.1:

type: object
properties:
  subtitle:
    type:
      - string
      - 'null'

Het verschil is dat het type attribuut een array van data types mag bevatten i.p.v. een enkele waarde. Zie ook de release notes van OAS v3.1:

The nullable keyword has been removed from the Schema Object (null can be used as a type value).

Om te voorkomen dat er verschillende schema components nodig zijn voor verschillende HTTP methods zou gebruik kunnen worden gemaakt van de eerder genoemde constructie:

application/merge-patch+json:
  type: object
  properties:
    $ref: '#/components/schemas/Book/properties'
joeribekker commented 3 years ago

De oorspronkelijke vraag was:

  1. Mag je bij een PATCH tekst [gemarkeerd als required] toch weglaten omdat PATCH sowieso al impliceert dat ik slechts enkele properties wil updaten?

Ondertussen zijn we bijna 2 jaar verder, en ook weer wat wijzer geworden :)

JSON schema heeft geen kennis van de bovenliggende standaard OAS. En, OAS overruled niet het gedrag van JSON schema (zoals required) op basis van een bepaalde HTTP-method zoals PATCH. Dit impliceert dat PATCH-requests dus gewoon alle required properties dient mee te geven, anders valideert het request niet (voldoet niet aan het JSON schema).

De enige mogelijkheid om PATCHes toe te staan waarin het wel of niet opnemen van properties volledig vrij is, is om een afwijkende requestBody te specificeren voor PATCH-requests (met typisch alle properties uit een POST request) waarbij minder (of geen enkele) properties required zijn. Op deze manier kan je PATCH-requests afvuren zonder de properties op te nemen die in een POST wel required zouden zijn. Of dit met een afwijkend media type moet vind ik zelf minder interessant/nodig aangezien je voor een OAS in alle gevallen een afwijkende body moet definieren. (maar dit is een andere discussie).

Destijds twijfelde ik of PATCH alle required-regels negeert (vandaar de originele post), en hoewel dat vriendelijker zou zijn voor het aantal models (en daarmee ook eenvoudigere code en schemas oplevert), is dat gewoon niet het geval in OAS 3.

Deze discussie wordt overigens ook gevoerd bij het team achter OAS, bijvoorbeeld: https://github.com/OAI/OpenAPI-Specification/issues/2201 en https://github.com/OAI/OpenAPI-Specification/issues/1497

Nog 2 sidenotes:

joostfarla commented 3 years ago

@joeribekker, klopt helemaal!

Je kan voor GET en POST vaak nog wel dezelfde modellen gebruiken, door gebruik te maken van readOnly of writeOnly waardoor ze wel/niet in de body mogen voorkomen.

Dit is vanaf OAS v3.1 niet meer het geval. Uit de release notes:

Due to the compliance with JSON Schema, there is no longer interaction between required and readOnly/writeOnly in relation to requests and responses.

Voor de semantiek van readOnly/writeOnly wordt dus teruggevallen op de JSON Schema specificatie. Daarin wordt geen relatie meer gelegd met het gebruik van required attributen.