digdir / dialogporten

Dialogporten - common API and and metadata state store for digital dialogs
https://docs.altinn.studio/dialogporten
MIT License
1 stars 3 forks source link

Udefinerte felter i response istedenfor tomme lister eller tomme verdier #1355

Open prange opened 2 weeks ago

prange commented 2 weeks ago

Description

Ved kall til /serviceowner/dialogs er inneholder returobjektet ikke items når ingen dialoger er funnet. Forventningen min er at det er en tom liste da typen PaginatedListOfSearchDialogSO ikke har spesifisert at verdien er optional. (se https://docs.altinn.studio/dialogporten/reference/entities/dialog/#search-1)

Reproduction

GET /serviceowner/dialogs?externalReference=xyz

Expected behavior

Alle felter er satt i objektet slik det er definert i datatypen, enten med tomme lister eller evt null.

MagnusSandgren commented 2 weeks ago

I dag purger vi alle felter som er null eller tomme fra responsen uavhengig av om de er nullable i responsmodellen eller ikke. Det ble gjort i sin tid for å lette trafikken, men kan definitivt skape forvirring og gjøre terskelen for å bruke DP høyere.

Foreslår å kun fjerne felter fra responsen dersom feltet på responsmodellen er nullable.

elsand commented 2 weeks ago

Enig, vi fjerner IgnoreEmptyCollection filteret.

MagnusSandgren commented 2 weeks ago

Jeg tenkte å modifisere IgnoreEmptyCollection filteret til å kun gjelde nullable retur typer, slik at vi har funksjonaliteten dersom vi velger å uttrykke at noen av listene skal være nullable i returmodellene. Men du vil heller fjerne hele filteret @elsand? 🙂

elsand commented 2 weeks ago

Hm, hvis vi gjør om en List<T> til å være nullable, gir det da ikke mening å initialisere den til null i stedet for [], slik at den da blir fjernet med Serializer.Options.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull som vi allerede bruker? Eller mener du noe annet?

MagnusSandgren commented 2 weeks ago

Etter en diskusjon med @elsand og @oskogstad lander vi på å gjøre alle lister nullable i swagger output.

Rasjonale er oppsummert her:

Mitt forsøk på å strukturere tanker:

  1. Null liste gir i utgangspunktet ikke mening: En null liste gir egentlig ikke mening i mitt hode - sett bor i fra i en veldig spesifikk kontekst. Det blir litt som en nullable boolean. Og en sånn en gir kun mening som input i et søk - altså jeg har tatt stilling til det (true/false), eller jeg vil ikke at det skal ha noe utfall i søket mitt (null). Samme for en liste - enten bryr jeg meg om det og sender inn en liste, eller null om jeg ikke vil at søket mitt skal ta hensyn til det. I denne konteksten vil [] og null kunne ha forskjellig betydning - men ikke alltid.
  2. Null og tom output-liste må være ekvivalent: Finnes det scenarier hvor [] og null kan ha forskjellig betydning i en ren output kontekst? Jeg kommer ikke på noe i farten.
  3. Spare trafikk: Dersom 2 holder vann og vi bruker Serializer.Options.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull gir det mening å fjerne null - og dermed også tomme lister fra output.
  4. Ikke lyv for sluttbruker: Dersom 3 holder vann må all output ICollection bli nullable. Dekorere alle lister som nullable i swagger?
prange commented 1 week ago

Jeg tror at dersom dere vil spare på bytes som sendes over kabelen er det andre ting en å ta vekk felter som gir mer effekt - f.eks. mulighet for å definere et felt for idempotency, for å redusere antall GETs man må gjøre for å forhindre dobbeltkall.

Punktlisten over resonnerer godt i mitt hode - jeg vil allikevel slå et slag for at null og [] skaper ulik forventning hos klienten: Dersom en liste er nullable er både null, [] og [e1,...] mulige svar som klienten må håndtere. En utvikler må dermed alltid dobbeltsjekke hva forskjellen mellom null og [] er, og skrive kode som håndterer de to casene som samme. - Ikke mye, men mange bekker små...

oskogstad commented 1 week ago

De skal være nullable i tt02 nå