melsk-r / HC-BAG-bevragen-issues

0 stars 0 forks source link

zoeken met oppervlakte < 0 geeft geen fout #294

Closed melsk-r closed 4 months ago

melsk-r commented 4 months ago

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

https://api.bag.acceptatie.kadaster.nl/esd/huidigebevragingen/v1/adresseerbareobjecten?bbox=135207,457400,135418,457162&oppervlakte[min]=-1&oppervlakte[max]=-1

dit geeft resultaten, geen fout. oppervlakte[min] is kleiner dan nu, en ook oppervlakte[max]<0, maar dat kan in de werkelijkheid niet

@MelvLee hoe zou jij dit het liefst zien? een oppervlak kleiner dan 0 accepteren (geeft bij min alles≥0, geeft bij max<1 lege collectie), of zou er een minimum moeten worden gespecificeerd?

melsk-r commented 4 months ago

This comment originally might have been created by someone else.

Komen er oppervlaktes van 0 of kleiner dan 0 voor? Zo niet, dan lijkt het mij goed om een minimum te specificeren. Ik denk 1?

melsk-r commented 4 months ago

This comment originally might have been created by someone else.

De oppervlakte van een verblijfsobject is volgens de BAG Catalogus 2018: "Een natuurlijk getal tussen 1 en 999999".

melsk-r commented 4 months ago

This comment originally might have been created by someone else.

< 0 moet 400 opleveren met invalidparams minimum | Waarde is lager dan minimum {minimum}. | minimum

Waaarbij de minimum waarde dus 1 is.

melsk-r commented 4 months ago

This comment originally might have been created by someone else.

@strijm voeg jij deze conditie toe in de specs, zodat het voor de gebruiker duidelijk is dat een negatief oppervlakte niet mag?

melsk-r commented 4 months ago

This comment originally might have been created by someone else.

@strijm bij IntegerFilter is op min nog geen minimum: 0 toegevoegd. Het is dus nog toegestaan oppervlakte[min]=-1 te sturen

voer je de wijzigingen ook door in de code zodat er ook foutmeldingen komen op de beschreven condities?

melsk-r commented 4 months ago

This comment originally might have been created by someone else.

@strijm bij IntegerFilter is op min nog geen minimum: 0 toegevoegd. Het is dus nog toegestaan oppervlakte[min]=-1 te sturen

voer je de wijzigingen ook door in de code zodat er ook foutmeldingen komen op de beschreven condities?

Doordat IntegerFilter voor meerdere query parameters wordt gebruikt en er voor iedere parameter andere min of max waarden kunnen gelden, kan er geen minimale of maximale waarde worden gespecificeerd in IntegerFilter. Dat zou wel kunnen als er voor iedere parameter een apart filter wordt gemaakt bv.: BouwjaarFilter en OppervlakteFilter. In de code worden de grenswaarde die nu bij de query parameters in de API specifiatie staan wel gecontroleerd. De release staat nu op de test omgeving!

melsk-r commented 4 months ago

This comment originally might have been created by someone else.

Doordat IntegerFilter voor meerdere query parameters wordt gebruikt en er voor iedere parameter andere min of max waarden kunnen gelden, kan er geen minimale of maximale waarde worden gespecificeerd in IntegerFilter. Dat zou wel kunnen als er voor iedere parameter een apart filter wordt gemaak

@strijm in https://github.com/VNG-Realisatie/Haal-Centraal-BAG-bevragen/issues/477#issuecomment-953984849 geef je zelf ook aan dat er een minimum moet zijn en een 400 fout moet komen. Ook @MelvLee geeft aan dat dit een goed idee is. En in de implementatie werkt het wel zo dat er een minimum is gedefinieerd:

    "invalidParams": [
        {
            "name": "oppervlakte[min]",
            "code": "minimum",
            "reason": "Waarde is lager dan minimum 1."
        }
    ]

dus dan zou ik ook verwachten dat dit zo in de specificaties staat. vandaar dit issue. @strijm Pas jij dat aan in de specificaties?

melsk-r commented 4 months ago

This comment originally might have been created by someone else.

@fsamwel De minimum en maximum waarden staan in de description van oppervlakte en bouwjaar. Is dat niet voldoende? Moet dit per se met een minimum en maximum keyword in de parameter definitie?

melsk-r commented 4 months ago

This comment originally might have been created by someone else.

@strijm ik zou dat liever in de technische definitie hebben zodat een gebruiker niet aanvullende logica moet lezen in de descriptions en zelf moet implementeren. @MelvLee maakt het wat jou betreft uit?

melsk-r commented 4 months ago

This comment originally might have been created by someone else.

bij oppervlakte[min] krijg ik nu een fout. bij oppervlakte[max] nog niet. ook bij bouwjaar[min] en bouwjaar[max] is dit niet doorgevoerd

melsk-r commented 4 months ago

This comment originally might have been created by someone else.

ik heb ook een pull request #505 ingediend om de minimum waarden aan de specificaties toe te voegen. Dan is het voor een gebruiker direct duidelijk dat deze beperking geldt.