melsk-r / HC-BAG-bevragen-issues

0 stars 0 forks source link

Als gebruiker wil ik gesorteerde resultaten in combinatie van pagineren #257

Open melsk-r opened 2 months ago

melsk-r commented 2 months ago

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

...zodat een gebruiker bij veel resultaten gericht naar een item kan zoeken.

Bijvoorbeeld /adressen?pandIdentificatie=0014100022188747 Dit pand heeft 296 adressen. Dit wordt nu ongesorteerd geleverd, zodat het niet makkelijk is het juiste adres te vinden, of om te zien welke opeenvolgende adressen in het pand zitten. Daarvoor zou het zoekresultaat moeten worden gesorteerd.

De gebruiker kan dat niet, althans niet zonder eerst alle resultaten op te halen, die samen te voegen, dat te sorteren, en het vervolgens weer gesorteerd en gepagineerd aan de eingdgebruijker aan te bieden.

Voorstel is om oplopend te sorteren op straat, huisnummer, huisletter, huisnummertoevoeging.

Dit gaat over adressen, adressen/zoek en adresseerbareobjecten.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

@CathyDingemanse ben je het hiermee eens? Ik zie dit bijvoorbeeld in onze inzagefunctie, waar je een ongesorteerde lijst krijgt. Of moet de client (dus onze inzagefunctie) die oplossen?

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

Ik denk dat de wijze van sorteren het beste door de client zelf kan worden gedaan, omdat die weet wat het beste past bij de businesstoepassing. Die functionaliteit is niet agnostisch. @customerZero @MelvLee wat vindt jij?

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

Is de operatie idempotent? M.a.w. krijg ik voor dezelfde request (incl. paging parameters) altijd hetzelfde resultaat? Zo niet, dan zou je dat moeten fixen. Sorteren zou een fix kunnen zijn. Hoe groot kan pageSize zijn. Als consumer zou ik (als sortering niet via parameters kan worden opgegeven of door de provider is gedaan) zelf sortering moeten implementeren. In dat geval moet ik dan alle adressen moeten hebben om goed te kunnen sorteren

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

Nu ik verder over nadenk, dan denk ik dat paging zonder sortering functionaliteit eigenlijk niet compleet is. Als je paging hebt, dan wil je volgens mij bij grote sets dat het gezochte item zo dicht mogelijk aan het begin van de gevonden set staat. Als je de sortering niet kan beïnvloeden, dan is dit volgens mij moeilijk te realiseren

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

Ter overweging: Uit performance overwegingen moet je dan ook zorgen dat je alleen sorteert op geindexeerde gegevens-elementen. Anders moet de provider bij iedere vraag altijd alle voorkomens ophalen. Indexeren in het geval van adressen is complex omdat het adres een view is over een x aantal BAG-objecttypen.

Technisch kan het natuurlijk wel, maar een dergelijke vraag kan aan de provider kant een grote performance impact hebben.

Als je de sortering wil kunnen beïnvloeden vanuit de consumer betekent dat dat je een grotere set van indexen er op na moet houden. Niet onmogelijk, maar ook niet even tussen neus en lippen door geregeld.

Lijkt mij een goed idee om de discussie hierover met Mark en zijn team op te starten.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

Technisch kan het natuurlijk wel, maar een dergelijke vraag kan aan de provider kant een grote performance impact hebben.

Dat is een belangrijk aspect, dat @strijm wellicht kan bevestigingen of ons geruststellen. Maar nu is dat probleem volledig verplaatst naar de client. Dus een client die de adressen bij een pand wil tonen aan een gebruiker moet nu eerst alle adressen ophalen in (zo nodig) meerdere requests (maximale pageSize is 100) en dat resultaat sorteren. Als het in de database lang duurt om bijv. 300 adressen op te vragen en te sorteren, duurt het denk ik altijd langer om 3 requests naar 100 ongesorteerde adressen te doen en dat resultaat te sorteren. Wanneer in de gebruikersinterface ook gepagineerd wordt (de gebruiker krijgt een beperkt aantal adressen en de knoppen << en >> op het scherm te zien), dan moet de client (zeker als dit een webserver is) het hele resultaat bij de sessie persisteren, of bij elke nieuwe pagina de hele sortering (dus alle daarvoor benodigde requests) opnieuw doen.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

Dus een client die de adressen bij een pand wil tonen aan een gebruiker moet nu eerst alle adressen ophalen in (zo nodig) meerdere requests (maximale pageSize is 100) en dat resultaat sorteren

Belangrijk om mee te nemen is welk percentage gesorteerd moeten zijn. Als elke zoekactie gesorteerd moet worden, dan is het verstandig om het bij de provider op te lossen.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

Paginering zonder sortering is eigenlijk niet mogelijk. Op dit moment wordt er al gesorteerd op nummeraanduidingIdentificatie (bij het zoeken van adressen bij een pand ID). Hierdoor lijkt het wellicht alsof er niet wordt gesorteerd, maar dat klopt dus niet helemaal. Een andere sortering is wel mogelijk, bv. op postcode, huisnummer, huisletter, huisnummertoevoeging. In dat geval wordt er nog gesorteerd op gegevens binnen het nummeraanduiding object. Als er op straatnaam, huisnummer etc. gesorteerd moet worden, dan wordt er over gegevens van meerdere objecten (tabellen) gesorteerd.
Sorteren kost wel wat tijd en het is inderdaad afhankelijk van welke gegevens er worden gebruikt om op te sorteren. Als er over meerdere attributen per object, of over attributen in verschillende objecten wordt gesorteerd dan zal dat invloed hebben op de performance. De gegevens waarop wordt gesorteerd (of waarop wordt gezocht in de database) moeten inderdaad geïndexeerd zijn voor een goede performance. De nummeraanduidingIdentificatie waar nu op wordt gesorteerd is één enkel gegeven van één object en is (uiteraard) geïndexeerd. Sneller dan dat wordt het niet, oftewel als er op een andere manier wordt gesorteerd, dan zal dit invloed hebben op de performance. Omdat er nog geen specifieke requirements waren voor het sorteren van grote result sets met paginering, hebben we voor deze 'beste performance' oplossing gekozen.

In de NL API strategie design rules extension wordt gesproken over een (niet normatieve) 'sorteer' parameter (https://docs.geostandaarden.nl/api/API-Strategie-ext/#sorting), die door een client gebruikt kan worden om aan te geven op basis van welke gegevens de result set gesorteerd moet worden. Bij ieder 'zoek' endpoint kan gesorteerd worden op properties van de resource die via dat endpoint opgezocht kan worden. Op die manier vindt de sortering aan de provider kant plaats, maar wel op maat voor clients. Zoals hierboven al aangegeven, als er op alle resource properties of combinaties daarvan gezocht moet kunnen worden met de sorteer parameter, dan heeft dat impact op de performance, hoeveel dat zal zijn zouden we moeten meten. Daar durf ik op voorhand geen uitspraken over te doen. Wij zullen dan zeker nog de nodige technische aanpassingen moeten doen om een acceptabele performance te kunnen garanderen voor alle mogelijke zoek en sorteer acties.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

ik zie niet direct functionele behoefte aan het sorteren op iets anders dan de eerste adresregel van een adressering: straat + huisnummer + huisletter + huisnummertoevoeging.

Bijvoorbeeld waarom zou een gebruiker willen sorteren op adresseerbaarObjectIdentificatie, isNevenadres of geconstateerd? (voor de laatste twee zou dat meer lijken op het misbruiken van sorteren voor filteren).

Ik vrees ook dat introduceren van een sort parameter weer dezelfde complexiteit toevoegt als we al hebben met fields. Als je dat echt wilt kun je beter een graphQL API maken (op zich helemaal geen slecht idee, toch @MelvLee?). Hier gaat het om het relatief eenvoudig kunnen raadplegen van gegevens op een manier die ook eenvoudig op de juiste manier aan een eindgebruiker te presenteren zijn.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

Sorteren is meestal gelinkt aan zoeken/filteren. Als ik op een property waarde zoek, dan wil ik meestal minimaal ook op die property kunnen sorteren. Dit maakt het inderdaad allemaal complexer. Op dit moment is er nu geen vraag naar, dus zou ik ook wachten met sorting te specificeren. En als de vraag komt, dan kan inderdaad de conclusie zijn dat een GraphQL API beter voldoet.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

mijn conclusie/voorstel is nu dat we:

  1. nu geen behoefte hebben aan functionaliteit waarbij de gebruiker vrij is de sortering te kiezen of aan te passen (dus geen "sort" parameter)
  2. er nu wel behoefte is een standaard sortering te gebruiken die voor de gebruiker direct herkenbaar is (straatnaam, huisnummer, huisletter, huisnummer toevoeging)
melsk-r commented 2 months ago

This comment originally might have been created by someone else.

@fsamwel en @MelvLee, mee eens.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

betreft alleen adressen. adresseerbare objecten hebben geen adres, dus daar kan alleen op identificatie worden gesorteerd (andere wensen voor sortering hierop kennen we niet)

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

adressen/zoek sorteert al en kunnen we niet beïnvloeden

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

@CathyDingemanse @JohanBoer Het adressen endpoint is ook te bevragen met een adoID. Deze kan meerdere gerelateerde adressen hebben. Omdat dithetzelfde enpoint betreft wordt de sortering voor die bevraging ook aangepast zodat ze hetzelfde zijn. Mocht dat een probleem zijn, dan hoor ik het graag.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

@CathyDingemanse @JohanBoer Het adressen endpoint is ook te bevragen met een adoID. Deze kan meerdere gerelateerde adressen hebben. Omdat dithetzelfde enpoint betreft wordt de sortering voor die bevraging ook aangepast zodat ze hetzelfde zijn. Mocht dat een probleem zijn, dan hoor ik het graag.

super!

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

@strijm het is nu helemaal opgelost zoals afgesproken. Nadeel van de gekozen sortering is wanneer de huisnummertoevoeging numeriek is, dan is de sortering 1, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 2, 21, 22, 23, ...

Is het ook mogelijk om huisnummertoevoeging, wanneer deze volledig numeriek is, deze ook numeriek te sorteren?

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

Denk wel dat dat kan, ik ga het overleggen.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

@fsamwel Het betreft hier een alfanummeriek gegeven (zie ook BAG Catalogus 2018). Standaard sortering van alfanummerieke gegevens levert daardoor dit resultaat, dat is ook aangegeven tijdens één van de overleggen en dat was toen akkoord. Een andere manier van sorteren van nummerieke data in dit attribuut t.o.v. tekstuele data in hetzelfde attribuut is standaard niet mogelijk. Ons advies is dan ook om dit zo te laten.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

Sorteren is meestal gelinkt aan zoeken/filteren. Als ik op een property waarde zoek, dan wil ik meestal minimaal ook op die property kunnen sorteren. Dit maakt het inderdaad allemaal complexer. Op dit moment is er nu geen vraag naar, dus zou ik ook wachten met sorting te specificeren. En als de vraag komt, dan kan inderdaad de conclusie zijn dat een GraphQL API beter voldoet

Ik maak een GraphQL label aan.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

@CathyDingemanse @NicoleKortoomsBAG Heel goed om dit in de GraphQL wel mogelijk te (willen) maken (gezien label GraphQL), maar wat mij betreft kan de oplossing die nu is gebouwd wel naar productie. Is zoals die nu is al wel een relevante verbetering ten opzichte van wat nu in productie staat.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

@strijm @NicoleKortoomsBAG ik had de regressietest nog niet eerder gedraaid, nu wel. Krijg daar nu andere resultaten uit dan voorheen (was al weer een tijdje geleden dat ik deze test heb gedraaid). Kan het zijn dat er ook data is gewijzigd?

Bijvoorbeeld op https://api.bag.acceptatie.kadaster.nl/esd/huidigebevragingen/v1/adressen?pandIdentificatie=0014100022188747&page=6&pageSize=50 kreeg ik voorheen 46 adressen, nu 45.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

@NicoleKortoomsBAG @strijm er lijkt ook iets te zijn omgevallen wat betreft mogelijkOnjuist en wat betreft fields. Dat zou toch nu niet in deze release zitten?

/adressen/0018200000152130?fields=huisnummer,postcode,_links.openbareRuimte

Hier krijg geen mogelijkOnjuist, terwijl de openbareRuimte(Identificatie) mogelijkOnjuist is. Als ik fields=huisnummer,postcode,_links.openbareRuimte,openbareRuimteIdentificatie vraag krijg ik wel mogelijkOnjuist. In _links.openbareRuimte zit ook de openbareRuimteIdentificatie verwerkt. Ik dacht dat dit al wel goed werkte, want ik heb hier geen open issue voor.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

ehm, vorige punt (m.b.t. _links.openbareRuimte) was wel al gemeld, in https://github.com/VNG-Realisatie/Haal-Centraal-BAG-bevragen/issues/293#issuecomment-658193499

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

@fsamwel het is opgelost. Graag nog eens testen.

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

https://github.com/VNG-Realisatie/Haal-Centraal-BAG-bevragen/issues/375#issuecomment-832544473 is opgelost

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

@CathyDingemanse @NicoleKortoomsBAG Heel goed om dit in de GraphQL wel mogelijk te (willen) maken (gezien label GraphQL), maar wat mij betreft kan de oplossing die nu is gebouwd wel naar productie. Is zoals die nu is al wel een relevante verbetering ten opzichte van wat nu in productie staat.

Daar ben ik het mee eens. Jij ook @NicoleKortoomsBAG ?

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

@CathyDingemanse , ik ben het er zeker mee eens

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

Sortering is goed opgelost zoals gevraagd en afgesproken. Voor overige wensen (zie label GraphQL voor uitgebreidere sorteringsmogelijkheden) zet ik dit issue terug in de baaklog

melsk-r commented 2 months ago

This comment originally might have been created by someone else.

Is het niet handiger om dit issue dan af te sluiten en een nieuwe te maken voor uitgebreidere sorteringsmogelijkheden met Graphql?