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

wanneer er meerdere parameters fout zijn moet je ze allemaal tegelijk melden #503

Open
fsamwel opened this issue Nov 19, 2021 · 8 comments
Assignees
Labels
bug Something isn't working v1.4

Comments

@fsamwel
Copy link
Collaborator

fsamwel commented Nov 19, 2021

Wanneer meerdere parameters verkeerd zijn ingevuld moeten alle fouten in één keer worden gemeld. Anders zal de gebruiker alleen de gemelde fout herstellen en nog een keer een fout request sturen.

In foutafhandeling.feature staat hierover: "Wanneer er fouten zitten op meerdere parameters, wordt er per validatiefout een "invalidParams" instantie opgenomen in het antwoord. Alle fouten worden dus teruggegeven."

Bijvoorbeeld /panden?bbox=135207,457400,135418,457162,fout&bouwjaar%5Bmin%5D=fout&bouwjaar%5Bmax%5D=1940 geeft 1 invalidParams, terwijl ik er twee verwacht had (op bbox en op bouwjaar[min]):

    "invalidParams": [
        {
            "name": "bbox",
            "code": "number",
            "reason": "Waarde is geen geldig decimaal getal."
        }
    ]

Bijvoorbeeld /adressen?postcode=fout&huisnummer=fout geeft 1 invalidParams, terwijl ik er twee verwacht had (op postcode en op huisnummer):

    "invalidParams": [
        {
            "name": "huisnummer",
            "code": "integer",
            "reason": "Waarde is geen geldige integer."
        }
    ]

Bij een combinatie van een fout in een queryparameter plus een fout in een (crs) header zou ik dat ook graag zien, maar kan dat nu niet beide in invalidParams. Maar kan wel door beide titles samen te voegen. En/of door beide fouten in details te melden.
Bijvoorbeeld /panden?bbox=135228,457502,135246,457457,fout met request header 'Content-Crs: WGS84' geeft alleen de fout op bbox, niet op content-crs, ik had beide verwacht in title en detail:

{
    "type": "https://tools.ietf.org/html/rfc7231#section-6.5.1",
    "title": "Een of meerdere parameters zijn niet correct.",
    "status": 400,
    "detail": "Ongeldige waarde [135228,457502,135246,457457,fout] opgegeven voor parameter [bbox]",
    "instance": "https://api.bag.acceptatie.kadaster.nl/esd/huidigebevragingen/v1/panden?bbox=135228,457502,135246,457457,fout",
    "code": "paramsValidation",
    "invalidParams": [
        {
            "name": "bbox",
            "code": "number",
            "reason": "Waarde is geen geldig decimaal getal."
        }
    ]
}

N.B. dit kan niet wanneer de verschillende fouten verschillende http status codes geeft. Dus bijvoorbeeld wanneer een niet ondersteunde Content-CRS wordt gevraagd en ook een parameter fout is, kan je onmogelijk tegelijk een 400 (voor de parameter) en een 406 (voor de CRS) geven. In bovengenoemde gevallen kan dat wel, want het gaat allemaal om fouten onder statuscode 400.

@fsamwel
Copy link
Collaborator Author

fsamwel commented Dec 3, 2021

@strijm wat is hier nu precies opgelost? Er worden niet meerdere fouten tegelijk gemeld, zoals de foutafhandeling feature voorschrijft. wel is denk ik de voorrang regel van #498 doorgevoerd?

Bijvoorbeeld: /panden?bbox=135228,457502,135246,457457&bouwjaar[min]=fout&bouwjaar[max]=fout&statusPand=fout&geconstateerd=fout
deze geeft maar 1 invalidParams (statusPand)

@strijm
Copy link
Collaborator

strijm commented Dec 7, 2021

@CathyDingemanse
M.b.t. foutmelding hebben wij een probleem met het commiteren aan één van de requirements.

In de HC common foutafhandeling.feature staat dat als er meerdere parameters in een request een foutieve waarde bevatten, er één 400 (Bad request) response teruggegeven moet worden. Die foutmelding bevat dan één invalidParams sectie met daarin voor iedere foute parameter een melding met name, code, en reason.
Oftewel voor alle foutieve parameter waarden in een request wordt in de response voor iedere fout een melding gegeven.
De reden hiervoor is dat een gebruiker dan in één keer kan zien dat er meerdere fouten in een request zitten en niet meerdere foutmeldingen in aparte requests krijgt als er een fout is opgelost.

Dit is echter een bijzonder lastige requirement.
Deze manier van foutafhandeling is momenteel aan de providerkant niet zo geïmplementeerd. Op dit moment wordt de foutafhandeling functionaliteit in het springframework gebruikt, zodra deze een foute waarde in een request tegenkomt bij input validatie dan wordt er een fout response gegeven. Daarbij wordt er dus niet eerst naar alle parameters en waarden gekeken.
Als we de gewenste manier van foutafhandeling moeten implementeren, dan betekent dit dat wij de foutafhandeling van de API helemaal opnieuw moeten implementeren. Dit kan echter onmogelijk binnen twee dagen. Het heeft tevens impact op alle andere functionaliteit die al live staat, de kans dat hier regressie optreedt is aanzienlijk.
Ook kunnen wij op dat moment de gegenereerde code en de foutafhandeling functionaliteit in het springframework niet meer gebruiken en moeten de foutafhandeling volledig custom implementeren voor Haal Centraal. Vanuit onderhoudbaarheidsperspectief is dit voor ons onacceptabel.

Ik denk dat door deze requirement de implementatie aan de provider kant zo ingewikkeld wordt en wijzigingen in de toekomst zo kostbaar, dat daarmee het doel om clients een zo eenvoudig mogelijke API te bieden in geen verhouding staat tot de inspanning. Het gaat om een foutsituatie die zich niet zomaar voordoet.

Het betekent dat de client zich op dat moment niet aan de API specificatie houdt. Aangezien de client code gegenereerd wordt uit de API specificatie, is de kans dat er foutieve waarden in een request terecht komen vrij klein. Daarnaast moet er in een client input validatie geïmplementeerd worden om te voorkomen dat er door een gebruiker ingevoerde waarden worden opgestuurd naar de provider. Een client applicatie zal zelf het nodige aan validaties moeten doen om zich aan het contract (API specificatie) te houden.
Het betreft hier daarom waarschijnlijk met name fouten die tijdens ontwikkeling of bij handmatig gebruik zullen voorkomen. De complexiteit en kosten aan de provider kant staan m.i. in dit stadium dan niet in verhouding tot de winst die je hiermee behaalt. M.i. is het in dit stadium gerechtvaardigd om één foutmelding per keer te retourneren en niet één foutmelding met alle fouten.

M.i. is deze requirement in SMART termen dan ook niet wenselijk en op dit moment niet realistisch.
Verzoek is om deze requirement daarom te heroverwegen, mijn advies is om deze requirement te schrappen.
Om toch release 1.4 z.s.m. live te krijgen kunnen we, als je deze requirement toch wilt behouden, een known issue opnemen dat de foutafhandeling anders werkt dan gespecificeerd.

@CathyDingemanse
Copy link
Collaborator

Ok, dit kan dus niet generiek worden opgelost. Wij zullen de spec voor de BAG aanpassen.

@CathyDingemanse CathyDingemanse assigned fsamwel and unassigned strijm Dec 7, 2021
@fsamwel
Copy link
Collaborator Author

fsamwel commented Dec 7, 2021

@strijm soms worden nu meerdere fouten wel samen geretourneerd, bijvoorbeeld:

{
    "type": "https://tools.ietf.org/html/rfc7231#section-6.5.1",
    "title": "Een of meerdere parameters zijn niet correct.",
    "status": 400,
    "detail": "Bad request.",
    "instance": "https://api.bag.acceptatie.kadaster.nl/esd/huidigebevragingen/v1/adressen/zoek?zoek=plantage%20delft&page=0&pageSize=101",
    "code": "paramsValidation",
    "invalidParams": [
        {
            "name": "page",
            "code": "minimum",
            "reason": "Waarde is lager dan minimum 1."
        },
        {
            "name": "pageSize",
            "code": "maximum",
            "reason": "Waarde is hoger dan maximum 100."
        }
    ]
}

weet jij in welke situaties (dus waarom hier wel en op andere plekken niet) er wel meerdere fouten tegelijkertijd worden?

@fsamwel
Copy link
Collaborator Author

fsamwel commented Dec 7, 2021

of bijvoorbeeld: /adresseerbareobjecten?nummeraanduidingIdentificatie=0599200000193766&expand=geenResource&fields=geenProperty&pageSize=1000

    "invalidParams": [
        {
            "name": "expand",
            "code": "expand",
            "reason": "Deel van de parameterwaarde is niet correct: geenResource."
        },
        {
            "name": "fields",
            "code": "fields",
            "reason": "Deel van de parameterwaarde is niet correct: geenProperty."
        },
        {
            "name": "pageSize",
            "code": "maximum",
            "reason": "Waarde is hoger dan maximum 100."
        }
    ]

@strijm
Copy link
Collaborator

strijm commented Dec 8, 2021

Parameter fouten kunnen alleen worden gecombineerd in één foutmelding als het type van alle parameter waarden overeenkomt met het gespecificeerde type van die parameters.

Als één van de parameters bv. van type integer is of een array van integer en er wordt een letter opgegeven, dan treedt er een fout op bij het omzetten van de waarde naar het gespecificeerde type (parsing). Er treedt dan een foutmelding op dat het type van de waarde van die parameter niet juist is. In dat geval wordt er niet meer verder gekeken naar evt. andere parameters en kunnen foutmeldingen niet worden gecombineerd in één foutmelding.
In dit geval wordt er dus niet gevalideerd of de waarden van parameter conform specificatie zijn.
Hieronder een lijstje van parameters per endpoint waarvoor geldt dat als deze een waarde bevatten die niet overeenkomt met het gespecificeerde type, er een fout optreedt waarbij geen combinatie mogelijk is.

  • endpoint /adresseerbareobjecten: page, pageSize, geconstateerd, type, gebruiksdoelen, bbox, oppervlakte[min], oppervlakte[max]
  • endpoint /adressen/zoek: page, pageSize
  • endpoint /adressen: page, pageSize, huisnummer, exacteMatch
  • endpoint /panden, locatie, bbox, statusPand, geconstateerd, bouwjaar[min], bouwjaar[max], page, pageSize

Als de waarde van alle parameters wel van het juiste type zijn (parsing van alle parameter waarden is gelukt), dan kan er worden gevalideerd of de waarden conform specificatie zijn. Als één of meer waarden van parameters niet in de gespecificeerde waarde range liggen, dan kunnen fouten wel worden gecombineerd in één foutmelding.

@fsamwel
Copy link
Collaborator Author

fsamwel commented Dec 13, 2021

@strijm komt het bovenstaande lijstje niet overeen met alle parameters die gedefinieerd zijn als integer, number, Boolean of enumeratie? (strings kunnen immers altijd worden gesteriliseerd)

@strijm
Copy link
Collaborator

strijm commented Dec 13, 2021

Ja en arrays met deze types en objecten. Een URI is gewoon een string. De waarden van parameters moet afhankelijk van het type geparsed worden van een string naar het type zoals gespecificeerd in de API spec. De waarde van een string parameter hoeft dus niet geparsed te worden.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v1.4
Projects
None yet
Development

No branches or pull requests

3 participants