Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Brk bevragen1.1 #532

Closed
wants to merge 19 commits into from
Closed

Brk bevragen1.1 #532

wants to merge 19 commits into from

Conversation

JohanBoer
Copy link
Collaborator

@JohanBoer JohanBoer commented Jun 10, 2020

Dee pull request heb ik gedaan om de verschillen tussen de huidge master en de BRK-bevragen ++ versie inzichtelijk te maken. Het is niet de bedoeling dat deze pull request wordt gemerged.

… erfpachtcanon. Filiatie toegevoegd (ook embedded bij KOZ. Mandeligheid toegevoegd aan Zakelijkgerechtigde.
2 incorrecte verwijzingen hersteld
…agen daarin opgenomen (ook in de _links en de identificaties in de tenaamstelling).
Bij ZakelijkGerechtigde _embedded toegevoegd en ook hypotheken en besl…
Hypotheken en beslagen verwijderd uit de tenaamstelling en zakelijkge…
@fsamwel
Copy link
Collaborator

fsamwel commented Jun 11, 2020

@JohanBoer de domein property bij identificaties ontbreekt nu

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 11, 2020

In een zakelijkgerechtigde.tenaamstelling en in hypotheek en in beslag zit een aantekening, maar hier wordt ook "aantekening" gebruikt als een soort synoniem voor privaatrechtelijke beperkingen. Is de aantekening in een tenaamstelling en een privaatrechtelijke beperking hetzelfde? Anders zou ik de termen anders gebruiken.

zie #423 die vraagt om een specifieke beschrijving van aantekening in de verschillende situaties.

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 11, 2020

een privaatrechtelijke beperking heeft isGebaseerdOpStukdeelIdentificatie en isVermeldInStukdeelIdentificaties.

In de bijbehorende _links zit een link naar stuk, geen link naar deze stukdelen. Daarvoor hebben we wel een endpoint. Moeten deze dan niet ook worden toegevoegd als links?

zelfde bij publiekrechtelijke beperking

en iets vergelijkbaars bij isGebaseerdOpStukdeelIdentificatie. Daar is er wel een link naar stukdelen, maar is dan niet duidelijk of dit een gebaseerd of vermeld stukdeel betreft.

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 11, 2020

De description van isVermeldInStukdeelIdentificaties heeft een taalfout: "De identificaties van het stukdelen waarin deze aantekening is vermeld"

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 11, 2020

In KadastraalOnroerendeZaak_links zit een link publiekrechtelijkeBeperkingen. Dat is een array, wat suggereert dat hierin de links komen naar de verschillende publiekrechtelijke beperkingen.
Dan is er ook een endpoint /kadastraalonroerendezaken/{kadastraalonroerendezaakidentificatie}/publiekrechtelijkebeperkingen/{identificatie} nodig. Alternatief is om deze link niet als array te definiëren.

Waarom is er eigenlijk geen link naar privaatrechtelijke beperkingen vanuit de KOZ?

Ook ontbreken de identificaties (naar publiekrechtelijke- en privaatrechtelijke beperkingen) in de resource.

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 11, 2020

in publiekrechtelijkeBeperkingen.beperkingsgebied.werkingsgebied zit property "kadatstraalObjectIdentificatie". Moet denk ik zijn "kadastraalObjectIdentificatie"

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 11, 2020

In KadastraalOnroerendeZaak_links en KadastraalOnroerendeZaak_embedded zitten de links/resources onroerendeZaakFiliatie en voorwaartseOnroerendeZaakFiliatie.

Naar welk endpoint verwijzen deze links? Zo te zien is dit geen resource.

Waarom is dit in embedded opgenomen? Zijn dit niet gewoon eigenschappen van de KOZ, en geen eigen resource? Deze bevat immers maar 4 properties, geen _links (zou nog best logisch geweest zijn, want er zitten kadastraalOnroerendeZaakIdentificaties in).

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 11, 2020

Volgens imKad heeft een zekerheidsstelling een relatie met een hypotheeknemer. En vanuit de aantekening naar de betrokken persoon. In de resource hypotheek is er een relatie met een hypotheekhouder. Die is in de specificaties niet verder gedefinieerd. Welke is dit? De geldverstrekker (bank) of de koper van woning? En wat is gebeurd met de relatie naar de andere betrokken persoon?

De wet en het BRK redeneren andersom dan in de boze buitenwereld gebruikelijk is, dus is het belangrijk dit erg duidelijk te beschrijven.

N.B. valt me op dat ook in inKAD het niet nodig gevonden werd om hypotheeknemer te definiëren. Tja...

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 11, 2020

Hypotheken kunnen worden gezocht op de persoon. Dit is, als ik de endpoint description goed begrijp, de Hypotheekhouder. Kunnen we die properties dan niet beter hypotheekhouder__burgerservicenummer, hypotheekhouder__kadasterpersoonidentificatie en hypotheekhouder__typepersoon noemen?

idem bij beslagen.

De parameternamen in de endpoint description en de feitelijke parameternamen moeten ook worden gelijkgetrokken. (zowel bij hypotheken als bij beslagen)

Voor persoon__typepersoon is geen enumeratie gedefinieerd, dus het is niet duidelijk wat hier ingevuld moet worden. Hier kan gebruik worden gemaakt van PersoonType_enum zoals die in /beslagen wel wordt gebruikt.

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 11, 2020

begrijp ik het goed dat hypotheken en beslagen alleen kunnen worden gezocht op resp. hypotheeknemer (de geldverstrekker) en beslaglegger, en niet op hypotheekgever (woningeigenaar)? Vult dit dan de functionele wens in van #264?

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 11, 2020

beslagen is gemaakt (volgens goals canvas) t.b.v. 2 user stories:

@CathyDingemanse vult zoeken op beslagleggers zonder periode #130 in?
En hoe biedt zoeken op beslagleggers inzicht in relaties van eigenaren?

@MelvLee
Copy link
Collaborator

MelvLee commented Jun 12, 2020

Ik denk dat het goed zou zijn om properties die horen bij een functionaliteit te groeperen en in specificieke componenten te stoppen. Dit helpt bij het scheiden van verantwoordelijkheid en maakt het ook duidelijker waar de properties voor bedoeld zijn.

Een voorbeeld: We hebben nu de KadastraalOnroerendeZaakHal die inherit van KadastraalOnroerendeZaak met _links en _embedded properties. Ik denk dat de stukIdentificaties, en zakelijkGerechtigdenIdentificaties ook in KadastraalOnroerendeZaakHal horen omdat deze nodig zijn om de templated urls in _links te kunnen expanden.

Ook helpt dit bij evolvability. Components kunnen namelijk onafhankelijk van elkaar evolven. Nu moet voor elke sub-resource die wordt gelinkt en geembed de ge-inherit resource worden uitgebreid met een identificaties array.

Om de API's ook onafhankelijk van elkaar te kunnen laten evolven is het denk ik nodit om de BRK core/common componenten in een eigen yaml te stoppen en daardoor misschien ook een eigen repo.
Nu zijn OnroerendeZaakFiliatie, VoorwaartseOnroerendeZaakFiliatie en PubliekrechtelijkeBeperking toegevoegd omdat deze nodig zijn voor de BRK Events/Update API. Zoals het nu is opgezet moet er van de BRK bevragen API een release worden gemaakt, als er van de BRK Events/Update API een release wordt gemaakt.

@MelvLee
Copy link
Collaborator

MelvLee commented Jun 12, 2020

verkavelObject in KadastraalOnroerendeZaak is nu gedefinieerd als een nested object. Het is beter om dit als een component te definieren en te refereren

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 12, 2020

Ik denk dat de stukIdentificaties, en zakelijkGerechtigdenIdentificaties ook in KadastraalOnroerendeZaakHal horen omdat deze nodig zijn om de templated urls in _links te kunnen expanden.

We hebben deze identificaties oorspronkelijk opgenomen voor gebruikers die de HAL links niet gebruiken, maar zelf de request naar de gerelateerde resource willen samenstellen. Dus deze moeten niet in de *Hal component komen.

@MelvLee
Copy link
Collaborator

MelvLee commented Jun 12, 2020

Ik denk dat de stukIdentificaties, en zakelijkGerechtigdenIdentificaties ook in KadastraalOnroerendeZaakHal horen omdat deze nodig zijn om de templated urls in _links te kunnen expanden.

We hebben deze identificaties oorspronkelijk opgenomen voor gebruikers die de HAL links niet gebruiken, maar zelf de request naar de gerelateerde resource willen samenstellen. Dus deze moeten niet in de *Hal component komen.

Dat herinner ik me nu ook. Maar dat is niet handig voor de Events/Update API, want dan krijgt je daar alle identificaties terwijl je die niet nodig hebt/zal gebruiken omdat je bij de Events/Update API de hele state van een resource krijgt

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 12, 2020

Maar dat is niet handig voor de Events/Update API, want dan krijgt je daar alle identificaties terwijl je die niet nodig hebt/zal gebruiken omdat je bij de Events/Update API de hele state van een resource krijgt

Heb je voor de update API niet ook behoefte aan het meekrijgen van de relaties? Maken de actuele relaties van een object/resource naar een ander object/resource niet deel uit van de state?

@MelvLee
Copy link
Collaborator

MelvLee commented Jun 12, 2020

Maar dat is niet handig voor de Events/Update API, want dan krijgt je daar alle identificaties terwijl je die niet nodig hebt/zal gebruiken omdat je bij de Events/Update API de hele state van een resource krijgt

Heb je voor de update API niet ook behoefte aan het meekrijgen van de relaties? Maken de actuele relaties van een object/resource naar een ander object/resource niet deel uit van de state?

Maar dat is niet handig voor de Events/Update API, want dan krijgt je daar alle identificaties terwijl je die niet nodig hebt/zal gebruiken omdat je bij de Events/Update API de hele state van een resource krijgt

Heb je voor de update API niet ook behoefte aan het meekrijgen van de relaties? Maken de actuele relaties van een object/resource naar een ander object/resource niet deel uit van de state?

Volgens mij krijgt je bij een wijziging alle interne relaties (zakelijkgerechtigden, stukken) mee. Dus van die lijkt het mij nutteloos om de identificaties daarvan mee te sturen.

Dit versterkt wel mijn gevoel om een BRK common/domain yaml te hebben, zodat je veel makkelijker kan inheriten en API's niet belast met elkaars requirements

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 12, 2020

Adres opnemen in de resource i.p.v. embedded. Reden: we kunnen ook zoeken op adres. Voor veel gebruikers is het adres interessant.
Is wel schending van principe "halen bij de bron", zoals we ook bij naam (omschrijving) en adres van personen hebben gedaan. Zijn comfortgegevens.
Adres ook behouden als link.

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 12, 2020

"locatieKadastraalObject" wordt "adres", waarin de adresgegevens en koppelingswijze (voorlopig) worden opgenomen

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 12, 2020

in zakelijk gerechtigde zit nu isOntstaanUitAppartementsrechtSplitsing bevat nu verenigingVanEigenaren. Deze is gedefinieerd als persoonBeperkt. Moet zijn NietNatuurlijkPersoonBeperkt, zodat de enumeratie van type klopt

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 12, 2020

verkavelObject in KadastraalOnroerendeZaak is nu gedefinieerd als een nested object. Het is beter om dit als een component te definieren en te refereren

Hetzelfde zie ik in:

  • Hypotheek_links.hypotheekhouder
  • PubliekrechtelijkeBeperking.beperkingsgebied
  • ZakelijkGerechtigde.mandeligheid
  • Beslag_links.beslaglegger

En hetzelfde hadden en hebben we in de *Collectie._embedded:

  • KadastraalOnroerendeZaakHalCollectie._embedded
  • HypotheekHalCollectie._embedded
  • KadasterNietNatuurlijkPersoonHalCollectie._embedded
  • BeslagHalCollectie._embedded
  • PubliekrechtelijkeBeperkingHalCollectie._embedded
  • ZakelijkGerechtigdeHalCollectie._embedded
  • KadasterNatuurlijkPersoonHalCollectie._embedded
  • PrivaatrechtelijkeBeperkingHalCollectie._embedded

@MelvLee is het niet erg wanneer we een object in een object definiëren bij de collecties?

  KadastraalOnroerendeZaakHalCollectie:
     type: "object"
     properties:
       _links:
         $ref: "https://raw.githubusercontent.com/VNG-Realisatie/Haal-Centraal-common/v1.0.0/api-specificatie/common.yaml#/components/schemas/HalCollectionLinks"
       _embedded:
         type: "object"
         properties:
           kadastraalOnroerendeZaken:
             type: "array"
             items:
               $ref: "#/components/schemas/KadastraalOnroerendeZaakHal"

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 12, 2020

In KadasterStuk zit property OorspronkelijkStukIdentificatie. Dit is niet lowerCamelCase.

@MelvLee
Copy link
Collaborator

MelvLee commented Jun 12, 2020

verkavelObject in KadastraalOnroerendeZaak is nu gedefinieerd als een nested object. Het is beter om dit als een component te definieren en te refereren

Hetzelfde zie ik in:

* Hypotheek_links.hypotheekhouder

* PubliekrechtelijkeBeperking.beperkingsgebied

* ZakelijkGerechtigde.mandeligheid

* Beslag_links.beslaglegger

En hetzelfde hadden en hebben we in de *Collectie._embedded:

* KadastraalOnroerendeZaakHalCollectie._embedded

* HypotheekHalCollectie._embedded

* KadasterNietNatuurlijkPersoonHalCollectie._embedded

* BeslagHalCollectie._embedded

* PubliekrechtelijkeBeperkingHalCollectie._embedded

* ZakelijkGerechtigdeHalCollectie._embedded

* KadasterNatuurlijkPersoonHalCollectie._embedded

* PrivaatrechtelijkeBeperkingHalCollectie._embedded

@MelvLee is het niet erg wanneer we een object in een object definiëren bij de collecties?

  KadastraalOnroerendeZaakHalCollectie:
     type: "object"
     properties:
       _links:
         $ref: "https://raw.githubusercontent.com/VNG-Realisatie/Haal-Centraal-common/v1.0.0/api-specificatie/common.yaml#/components/schemas/HalCollectionLinks"
       _embedded:
         type: "object"
         properties:
           kadastraalOnroerendeZaken:
             type: "array"
             items:
               $ref: "#/components/schemas/KadastraalOnroerendeZaakHal"

De code generatoren genereren een class met de naam _embedded. Als er meerdere _embedded op deze manier zijn gedefinieerd, dan krijgt je classes met de naam _embedded gevolgd door een nummer. Dus _embedded2, enz.

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 15, 2020

De code generatoren genereren een class met de naam _embedded. Als er meerdere _embedded op deze manier zijn gedefinieerd, dan krijgt je classes met de naam _embedded gevolgd door een nummer. Dus _embedded2, enz.

@MelvLee mag ik hieruit concluderen dat dit inderdaad ook onwenselijk is? Deze constructie gebruiken we namelijk ook in de BRP en BAG API definities.

@MelvLee
Copy link
Collaborator

MelvLee commented Jun 15, 2020

De code generatoren genereren een class met de naam _embedded. Als er meerdere _embedded op deze manier zijn gedefinieerd, dan krijgt je classes met de naam _embedded gevolgd door een nummer. Dus _embedded2, enz.

@MelvLee mag ik hieruit concluderen dat dit inderdaad ook onwenselijk is? Deze constructie gebruiken we namelijk ook in de BRP en BAG API definities.

Het is inderdaad niet wenselijk

@JohanBoer
Copy link
Collaborator Author

Het is inderdaad niet wenselijk

Als we dit gaan aanpassen dan zijn dat breaking changes. Gaan we die direct doorvoeren ?
Ik stel voor om deze wijziging op de backlog te zetten (bij alle API's) en bij de eerste breaking change op content ook door te voeren.
@CathyDingemanse Hoe denk jij daar over ?

@MelvLee
Copy link
Collaborator

MelvLee commented Jun 15, 2020

Het is inderdaad niet wenselijk

Als we dit gaan aanpassen dan zijn dat breaking changes. Gaan we die direct doorvoeren ?
Ik stel voor om deze wijziging op de backlog te zetten (bij alle API's) en bij de eerste breaking change op content ook door te voeren.
@CathyDingemanse Hoe denk jij daar o

De code generatoren genereren een class met de naam _embedded. Als er meerdere _embedded op deze manier zijn gedefinieerd, dan krijgt je classes met de naam _embedded gevolgd door een nummer. Dus _embedded2, enz.

@MelvLee mag ik hieruit concluderen dat dit inderdaad ook onwenselijk is? Deze constructie gebruiken we namelijk ook in de BRP en BAG API definities.

Ik heb even gekeken waarom deze issue niet speelt bij BRP en BRK. Ik zie dat in de gegenereerde variants deze constructie is vervangen door een class. Dat betekent dus dat de resolver dit doet. Als test heb ik van de BRKBevragen1.1 OAS een geresolvede versie gegenereerd en ook hier zie ik dat de resolver deze constructies vervangt met een class

@CathyDingemanse
Copy link
Collaborator

Ik denk dat we dit nu in v1.1 moeten doen. Breaking changes (in gedrag denk nen3610) zijn vanaf livegang aangekondigd. Er zijn nog geen afnemers in productie. Dat gebeurt met deze aankondiging sowieso pas in juli.

@MelvLee
Copy link
Collaborator

MelvLee commented Jun 15, 2020

Het is inderdaad niet wenselijk

Als we dit gaan aanpassen dan zijn dat breaking changes. Gaan we die direct doorvoeren ?
Ik stel voor om deze wijziging op de backlog te zetten (bij alle API's) en bij de eerste breaking change op content ook door te voeren.
@CathyDingemanse Hoe denk jij daar over ?

Ik denk dat het niet nodig is om dit asap door te voeren omdat de resolver dit al doet.

@fsamwel
Copy link
Collaborator

fsamwel commented Jun 15, 2020

Het is inderdaad niet wenselijk

Als we dit gaan aanpassen dan zijn dat breaking changes. Gaan we die direct doorvoeren ?

@MelvLee zijn dit breaking changes? Er wijzigt toch niks in de interface, alleen in de onderliggende componentenstructuur?

@JohanBoer
Copy link
Collaborator Author

Ik denk dat het niet nodig is om dit asap door te voeren omdat de resolver dit al doet.

Volgens mij hebben we voor de "gewone" _ links en _embedded gedaan voordat we de resolver gingen gebruiken. Als de resolver dit probleem oplost, moeten we hier dan überhaupt iets aan aanpassen ?

@MelvLee zijn dit breaking changes? Er wijzigt toch niks in de interface, alleen in de onderliggende componentenstructuur?

Het lijkt mij dat als de componenten structuur gewijzigd wordt dat dan de namen van je classes wijzigen. Dat is waarom ik denk dat het breaking is, maar ik ben erg benieuwd naar Melvins conclusie.

@MelvLee
Copy link
Collaborator

MelvLee commented Jun 15, 2020

Ik denk dat het niet nodig is om dit asap door te voeren omdat de resolver dit al doet.

Volgens mij hebben we voor de "gewone" _ links en _embedded gedaan voordat we de resolver gingen gebruiken. Als de resolver dit probleem oplost, moeten we hier dan überhaupt iets aan aanpassen ?

@MelvLee zijn dit breaking changes? Er wijzigt toch niks in de interface, alleen in de onderliggende componentenstructuur?

Het lijkt mij dat als de componenten structuur gewijzigd wordt dat dan de namen van je classes wijzigen. Dat is waarom ik denk dat het breaking is, maar ik ben erg benieuwd naar Melvins conclusie.

Het zijn geen breaking changes, alleen de namen van de onderliggende classes wijzigen. Wat de resolver doet is als hij een object tegenkomt als property dat hij deze in component. De naam van de component wordt dan de 'naam van de parent component' + '_' + 'naam van de property (idg _embedded)'. De code generator doet in principe hetzelfde, alleen gebruikt deze niet de naam van de parent als prefix maar wordt een ophogende nummer gebruikt als postfix.

Omdat het geen breaking change is zou ik het nu nog niet gaan aanpassen bij de BRP, maar wel bij de BRK (er komt een 1.1) en BAG (nog niet in productie). Dan is voor altijd de naamgeving van de _links en _embedded componenten gelijk en niet toevallig omdat de resolver het op dezelfde manier doet.

@JohanBoer
Copy link
Collaborator Author

Omdat het geen breaking change is zou ik het nu nog niet gaan aanpassen bij de BRP, maar wel bij de BRK (er komt een 1.1)

Het is inderdaad geen breacking change, maar heeft wel impact op de provider-implementatie. Die impact kan ik niet zo goed inschatten. Ik maak een aparte issue (#539) aan en vraag @rhengeveld om de impact in te schatten

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants