Skip to content

OS-245 updating CRV datafordeler endpoint#29

Merged
stankut merged 4 commits intomainfrom
f/cvr_datafordeler
Apr 14, 2026
Merged

OS-245 updating CRV datafordeler endpoint#29
stankut merged 4 commits intomainfrom
f/cvr_datafordeler

Conversation

@stankut
Copy link
Copy Markdown
Contributor

@stankut stankut commented Mar 30, 2026

Link to ticket

OS2Forms/os2forms#248

Description

Replacing the existing Datafordeler endpoint with a new endpoint based on GraphQL.

Additional comments or questions

This is a new of a kind Datafordeler webservice integration that is based on API key, and not the certificate as it used to.

That is why the architecture is changed a bit. Bear in mind, I deliberately decided not do put anything if the parent class and wait until have more example of the new Datafordeler webservices. After that the pattern will be more prominent, which will allow us to move the repetitive/shared features into the parent class

@Anna-itk
Copy link
Copy Markdown

Anna-itk commented Apr 7, 2026

@stankut will you please add jekuaitk as reviewer on this pr?

@stankut stankut requested a review from jekuaitk April 7, 2026 08:09
Copy link
Copy Markdown
Contributor

@jekuaitk jekuaitk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The before code allowed the use of a key. This is no longer the case

Other than that the GraphQL endpoint modifications looks good!

->format('Y-m-d\T00:00:00\Z');

$query = <<<GRAPHQL
{ CVR_Virksomhed(first: 1, virkningstid: "{$virkningstid}", where: { CVRNummer: { eq: {$cvr} } }) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we ensure/validate that this is indeed a CVR-number, i.e. 8 digits?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's do that! Fixed in 7f3bc6b

Comment thread CHANGELOG.md Outdated
Comment on lines +8 to +9
## [Unreleased]
* Datafordeler CRV service endpoint update - GraphQL.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## [Unreleased]
* Datafordeler CRV service endpoint update - GraphQL.
## [Unreleased]
* Datafordeler CVR service endpoint update - GraphQL.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of course.. 😄 fixed in 7f3bc6b

@stankut
Copy link
Copy Markdown
Contributor Author

stankut commented Apr 14, 2026

The before code allowed the use of a key. This is no longer the case

Other than that the GraphQL endpoint modifications looks good!

@jekuaitk the old implementation of the key module usage was limited to a custom key of type os2web_key_certificate, which in fact is a reference to the certificate located either locally or remotely (Azure support only).

For the this new GraphQL endpoint implementation none of that is relevant, as it is API key based. So a simple key string.

My plan was to keep it low profile for now, and use the simple logic to make it work, meaning just providing the API key as a string in the setting. As also mentioned above i would like to wait for another Datafordeler implementation before making any architectural decision/rules regarding the implementation. We need to see the common pattern. But when that comes, i think we would need to rewrite the DatafordelerBase by completely - geting rid of the hardcoded os2web_key_certificate and/or replace it to also support API key type of keys.

@jekuaitk
Copy link
Copy Markdown
Contributor

The before code allowed the use of a key. This is no longer the case
Other than that the GraphQL endpoint modifications looks good!

@jekuaitk the old implementation of the key module usage was limited to a custom key of type os2web_key_certificate, which in fact is a reference to the certificate located either locally or remotely (Azure support only).

For the this new GraphQL endpoint implementation none of that is relevant, as it is API key based. So a simple key string.

My plan was to keep it low profile for now, and use the simple logic to make it work, meaning just providing the API key as a string in the setting. As also mentioned above i would like to wait for another Datafordeler implementation before making any architectural decision/rules regarding the implementation. We need to see the common pattern. But when that comes, i think we would need to rewrite the DatafordelerBase by completely - geting rid of the hardcoded os2web_key_certificate and/or replace it to also support API key type of keys.

Makes sense. As long as we keep this is mind im good with this!

@stankut
Copy link
Copy Markdown
Contributor Author

stankut commented Apr 14, 2026

@jekuaitk thanks for a review, i went though the comments, please have a look again

@jekuaitk jekuaitk self-requested a review April 14, 2026 07:53
@stankut stankut merged commit 341825d into main Apr 14, 2026
4 checks passed
@stankut stankut deleted the f/cvr_datafordeler branch April 14, 2026 10:13
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.

Ændringer på datafordeler.dk - fremover skal den moderniserede Datafordeler benyttes

3 participants