search

melsk-r / HC-BAG-bevragen-issues

0 stars 0 forks source link

API voldoet niet aan algemene foutafhandeling feature #3

Closed melsk-r closed 3 months ago

melsk-r commented 3 months ago

Originally created by KayodeBakker (https://github.com/VNG-Realisatie/BAG-Gemeentelijke-wensen-tav-BAG-Bevragingen/issues/23):

Impediment

In de foutafhandeling feature, eerder in het leven geroepen via de BRP API, staat een specifieke opzet en beschrijving voor foutsituaties. Het valt op dat hier nog niet helemaal aan wordt gehouden.

Voorbeelden zijn: 404 waar title 'Opgevraagde resource bestaat niet.' hoort te zijn

{
    "status": 404,
    "title": "Not Found",
    "type": "about:blank",
    "code": "Geen verblijfsobject gevonden met id 0599010063000401"
}

400 waar title 'Een of meerdere parameters zijn niet correct.' hoort te zijn Hier is de reguliere expressie ook escaped waardoor een ontwikkelaar wellicht de reguliere expressie inclusief \ overneemt.

{
    "status": 400,
    "title": "Bad Request",
    "type": "about:blank",
    "code": "verblijfsobjectIdentificatie.identificatie: must match \"^[0-9]{4}01[0-9]{10}$\", verblijfsobjectIdentificatie.identificatie: size must be between 16 and 16"
}

404 waar title 'Opgevraagde resource bestaat niet.' hoort te zijn en zaken als timestamp, error, message en path niet deel uitmaken van de foutafhandeling feature.

{
    "timestamp": "2019-12-09T09:50:05.719+0000",
    "status": 404,
    "error": "Not Found",
    "message": "No message available",
    "path": "/v1/verblijfsobjecte"
}

In alle bovengenoemde voorbeelden zijn er ook consequent twee zaken verkeerd geïmplementeerd. Namelijk de type en code. De type is constant about:blank waar een referentie naar een beschrijving van de fout wordt verwacht. Zoals 'https://docs.microsoft.com/en-us/dotnet/api/system.net.httpstatuscode?#System_Net_HttpStatusCode_NotFound'. Code dient daarentegen niet een beschrijving te zijn, maar een code zoals: 'notFound' en 'paramsRequired'. Daarnaast missen we attributen als 'instance' en 'detail' en in geval van een 400 ook 'invalid-params'.

Ook krijgt de gebruiker constant een lege response met status 400 Bad Request terug als één van de volgende situaties worden uitgevoerd:

Als laatste zijn alle errors in het Engels terwijl de foutafhandeling feature Nederlands hanteert.

Graag hier naar kijken

melsk-r commented 3 months ago

This comment originally might have been created by someone else.

Bron: https://github.com/VNG-Realisatie/Haal-Centraal-BRP-bevragen/blob/master/api-specificatie/common.yaml

melsk-r commented 3 months ago

This comment originally might have been created by someone else.

De common.yaml is zeker een locatie voor de fout objecten, echter de bron voor foutafhandeling beschrijvingen zijn te vinden hier.

melsk-r commented 3 months ago

This comment originally might have been created by someone else.

De yaml specificaties voor de LV BAG Individuele Bevragingen API volgen hier de NL API strategie (7.1.45 API-47). Hierin staat:

APIs should at least support the following HTTP status codes: 200, 201, 204, 304, 400, 401, 403, 405, 406, 409, 410, 415, 422, 429, 500, and 503.

Waar nodig worden er aanpassingen gemaakt aan de yaml specificaties zodat deze voldoen aan de NL API strategie en aan de genoemde bron voor foutafhandeling. M.a.w. de attributen komen overeen met de genoemde attributen en de attributen worden ingevuld zoals voorgesteld, waarbij de title in het Nederlands is. De yaml specificaties bevatten geen andere dan de genoemde attributen.

melsk-r commented 3 months ago

This comment originally might have been created by someone else.

Dit wordt voorlopig niet geïmplementeerd, met uitzondering van de 400 fouten. Ondertussen wordt nagedacht over:

melsk-r commented 3 months ago

This comment originally might have been created by someone else.

Volgens mij is deze issue achterhaald. We gebruiken de fout-definities die in de haal-centraal Common.yaml staan. Die hebben we aangevuld met de foutmelding die vanuit kadaster-perspectief nog ontbraken.

@CathyDingemanse Kan deze dan gesloten worden ?

melsk-r commented 3 months ago

This comment originally might have been created by someone else.

Het probleem met de 404 fout is achterhaald. Die is nu opgelost. Voor andere situaties voldoet foutafhandeling nog niet aan de NL API strategie.

Bijvoorbeeld vragen adres met niet-valide nummeraanduidingIdentificatie geeft problem+json antwoord (correct), maar hierin ontbreekt invalid-params. Er blijkt dus niet uit het foutbericht wat er precies niet goed is aan het request. Althans niet in het formaat zoals wij dat verwachten, conform NL API strategie.

melsk-r commented 3 months ago

This comment originally might have been created by someone else.

Ook foutmelding bij onjuiste api-key is niet volgens verwachting, hier wordt wel correct http statuscode 401 teruggegeven, maar het antwoord is geen problem+json, maar text/plain met de inhoud "Invalid API Key". Ik denk dat een consistente vorm van foutafhandeling over alle soorten fouten zeer gewenst is, en ook dat een consistente vorm van foutafhandeling over alle Haal Centraal API's gewenst is. Daarom is foutafhandeling ook opgenomen in de NL API strategie.

Wij gaan een stukje verder door ook foutteksten en foutcodes voor te schrijven, zoals dat voor BRP is gemaakt. Het feature dat dit beschrijft is ook in de Haal-Centraal-common opgenomen. De noodzaak daarvan ben ik iets minder zeker van.

melsk-r commented 3 months ago

This comment originally might have been created by someone else.

Ik begrijp dat het zeer gewenst is dat een consistente vorm van foutafhandeling over alle soorten fouten over alle HC API's zeer gewenst is. Doordat niet alle fouten in de applicatie worden afgehandeld, maar soms al daarvóór in bv. een API gateway, web of applicatie server, load balancer etc. worden afgevangen en doordat er verschillende organisatie zijn die API's aanbieden, die verschillende systemen gebruiken met ieder een eigen vorm van HTTP fout responses, wordt het wel erg lastig en misschien zelfs onmogelijk om dit consistent te doen voor alle HC API's.

melsk-r commented 3 months ago

This comment originally might have been created by someone else.

niemand heeft ooit gezegd dat het makkelijk is ;)

Ik denk dat het toch mogelijk moet zijn om bij de drie betrokken organisaties (RvIG, KvK en Kadaster) te zorgen dat de API's die ze aanbieden voldoen aan de NL API strategie. Staat immers op de pas-toe-of-leg-uit lijst. En misschien lukt het uiteindelijk niet overal voor alle foutsituaties. Maar dat hoeft ons er niet van te weerhouden de maximale inspanning te doen om het wél voor elkaar te krijgen.

melsk-r commented 3 months ago

This comment originally might have been created by someone else.

Klopt het dat we nu moeten voldoen aan: https://github.com/VNG-Realisatie/Haal-Centraal-common/blob/master/features/foutafhandeling.feature i.p.v. https://github.com/VNG-Realisatie/Haal-Centraal-BRP-bevragen/blob/master/features/foutafhandeling.feature?

melsk-r commented 3 months ago

This comment originally might have been created by someone else.

ja, je moet voldoen aan de versie in Haal-Centraal-common. Had jij relevante verschillen geconstateerd tussen beide?

melsk-r commented 3 months ago

This comment originally might have been created by someone else.

Niet veel spannends. De common versie heeft voor fields en expand parameter één regel meer en in de foutafhandeling is invalid-params hernoemd, verder geen grote verschillen. Het ging mij er meer om, dat ik naar de juiste specificaties verwijs in onze Jira tickets.

melsk-r commented 3 months ago

This comment originally might have been created by someone else.

wel relevant dus, want we verwachten invalidParams, niet invalid-params

melsk-r commented 3 months ago

This comment originally might have been created by someone else.

Ja, maar dat wisten we al want die wijziging hadden we er bij het resolven van de yaml een paar weken geleden al gratis bij gekregen. Daarnaast moeten we het stuk met invalidParams nog implementeren.