search

VNG-Realisatie / Haal-Centraal

Repository voor de Haal Centraal GitHub Pages Landingspagina's
https://vng-realisatie.github.io/Haal-Centraal/
Other
0 stars 3 forks source link

Fuzzy-search op adres restfull maken #74

Open JohanBoer opened 3 years ago

JohanBoer commented 3 years ago

De huidige endpoint voor fuzzy search op adres in de BAG-bevragen API is uitgevoerd in een tweetraps-raket. Er wordt op basis van de fuzzy-searche een API-string opgeleverd en met die string kan de consumer het daadwerkelijke adres ophalen.

Bekijk in hoeverre de specificaties aangeapst moeten worden om in plaats van een array adresstrings een collectie van adressen op te halen zodat dit een restful endpoint wordt.

JohanBoer commented 3 years ago
fsamwel commented 3 years ago

Parameter-component "zoek" wordt required: false (Heeft non-breaking impact op deprecated endpoint "/adressen/zoek")

Dit betekent dat "zoek" ook niet meer required is bij /adressen/zoek? Waarom is dat nodig? Je hebt er nu "last" van (hoewel niet breaking) dat je de parameter in een component stopt en die hergebruikt, terwijl je voorheen de parameter maar één keer gebruikte en deze nu op verschillende plekken verschillend gebruikt. In #364 was er namelijk ook de behoefte om op zoek een minLength en (vooral) een maxLength (van 255) te definiëren. Dat is toen niet gedaan omdat het breaking zou zijn. De API kan nu fouten op invoer geven terwijl die invoer wel voldoet aan de interface specificaties, dus optimaal is dat niet. Het verplaatsen van de zoekparameter naar /adressen geeft de kans dat alsnog goed te doen.

Dus mijn voorstel is om de ouder parameter, of de nieuwe parameter (of beide) te definiëren bij het endpoint, dan wel voor de nieuwe parameter ook een nieuwe parametercomponent te maken.

MelvLee commented 3 years ago

Als een nieuwe parameter wordt gemaakt, dan stel ik voor om het dan ook een goede naam te geven. zoekTerm of iets in die geest

fsamwel commented 3 years ago

De API design rules extensions noemt deze parameter "zoek" (niet normatief): "APIs support full-text searching using the query parameter zoek."

In BRK noemen we deze "q" (zoeken kadasterpersonen).

Is het dan handig hier een derde parameternaam (zoekTerm) voor te verzinnen?

Ik ben het trouwens wel met Melvin eens dat "zoekTerm" duidelijker is dan "zoek" of "q".

MelvLee commented 3 years ago

q is de naam die door andere zoek engines wordt gebruikt. Zie bijv. https://www.elastic.co/guide/en/elasticsearch/reference/current/search-search.html. Mijn suggestie was toen om dan dezelfde benaming te gebruiken. Dus wat mij betreft mag q ook. Maar aangezien we nu als best practice hanteren om duidelijke namen te kiezen voor properties en parameters, is denk ik zoekTerm een betere benaming. Als we hiervoor kiezen, kunnen we "q" in kadasterpersonen deprecaten.

JohanBoer commented 3 years ago

Dit betekent dat "zoek" ook niet meer required is bij /adressen/zoek? Waarom is dat nodig? Je hebt er nu "last" van (hoewel niet breaking) dat je de parameter in een component stopt en die hergebruikt, terwijl je voorheen de parameter maar één keer gebruikte en deze nu op verschillende plekken verschillend gebruikt. In #364 was er namelijk ook de behoefte om op zoek een minLength en (vooral) een maxLength (van 255) te definiëren. Dat is toen niet gedaan omdat het breaking zou zijn. De API kan nu fouten op invoer geven terwijl die invoer wel voldoet aan de interface specificaties, dus optimaal is dat niet. Het verplaatsen van de zoekparameter naar /adressen geeft de kans dat alsnog goed te doen.

Dus mijn voorstel is om de ouder parameter, of de nieuwe parameter (of beide) te definiëren bij het endpoint, dan wel voor de nieuwe parameter ook een nieuwe parametercomponent te maken

Als ik het goed heb zal een developer die een nieuwe consumer-implementatie doet niet kiezen voor de deprecated zoek-functie. Dat betekent dat alle bestaande implementaties gewoon de zoek-parameter zullen vullen voordat er een request gedaan wordt. In feite wordt de component bij de eerstvolgende versie niet meer hergebruikt omdat dan de deprecated onderdelen verwijderd worden en deze parameter dus maar op 1 plek wordt gebruikt.

Ik heb de bestaande zoek-parameter hergebruikt om zo dicht mogelijk bij de huidige implementaties te blijven. Natuurlijk kunnen we nu minLength en maxLength toevoegen, maar aangezien #364 gesloten is had ik dat niet op mijn netvlies. \

Ten aanzien van het hernoemen van de "zoek"-parameter heb ik nergens een aanleiding voorbij zien komen om die naam nu aan te passen. De discussie over "q" of "zoek" hebben we in het ontwikkeltraject ook met het BAG-team ook gevoerd en daar is "zoek" uitgekomen. Als we roepen dat we ons conformeren aan de landelijke API_strategie dan zie ik niet in waarom we van "zoek" nu "zoekTerm" moeten maken. Ik snap dat "zoek" niet normatief is, maar waarom zou je afwijken als dat geen toegevoegde waarde heeft.

Dat gezegd hebbende. Ik ben geen productowner. De wijzigingen zijn nu zo doorgevoerd om zo dicht mogelijk bij de huidige implementatie te blijven. Het toevoegen van maxLength en minLength likt mij een goed idee. Een andere naam voor de zoek-parameter vind ik geen goed idee.

@CathyDingemanse Kan/wil jij uitsluitsel geven of hebben we het hier maandag over ?

JohanBoer commented 3 years ago

We gaan q gebruiken en daar voegen we maxLength en minLength toegevoegd.