/status API endpoint overrides configured endpoint

gbv / cocoda-sdk

SDK for Cocoda and coli-conc services

https://gbv.github.io/cocoda-sdk/

MIT License

5 stars 1 forks source link

/status API endpoint overrides configured endpoint #21

Closed nichtich closed 3 years ago

nichtich commented 4 years ago

Given a configuration

{
    "provider": "ConceptApi",
    "api": "http://example.org/",
    "schemes": "http://example.com/voc"
}

The schemes endpoint will be ignored but overridden by what is returned by /status endpoint.

I think this and #20 could be solved by filling this._api only after initialization with values from /status in _setup instead of _prepare.

stefandesu commented 4 years ago

I see your point. There are several different issues here though:

I agree that existing values in the configuration should definitely not be overridden by either assumed endpoints or the result of /status.
Then there's the question if any endpoints should even be assumed. My thought was that if we assume missing values, it would be easier to set up for example DANTE without having to explicitly naming every endpoint. On the other hand, assuming the endpoints also assumes that those endpoints even exist (which is the issue behind #20).
I would think that if there is a response on /status, we don't need to assume any another values. But /status itself has to be assumed if only api is given.

What do you think?

nichtich commented 4 years ago

A solved my use case for this issue by putting jskos-server instance behind a proxy so I could change endpoint base URIs. We could document the current behavior:

all endpoints are automatically set when api is given. Explicitly disable an endpoint by setting an it to undef (not tested, should solve #20)
when a status endpoint is set (explicitly or implicitly via api) it's return value will override all endpoints.

stefandesu commented 4 years ago

That's a good solution. Would you be opposed to using null instead of undefined for explicitly disabling an endpoint? It's much easier to check: status.type === null vs status.hasOwnProperty("type") && status.type === undefined.

nichtich commented 4 years ago

Nevermind null!

stefandesu commented 3 years ago

So, it is now implemented this way:

Explicitly set endpoints always have priority and will not get overridden (even if they are null).
- This allows us to override or disable endpoints if necessary for some reason.
Endpoints that are then still undefined will be set with the result of the status endpoint.
Endpoints that are then still undefined will be implicitly set if the api key is given.

Also, if the request to /status returns explicitly with a 404 error (i.e. it's not a network issue or some other server issue), we'll define that status endpoint as null.

Together with https://github.com/gbv/jskos-server/issues/119, this should now work as expected.

stefandesu commented 3 years ago

We could change one more thing @nichtich, if you want: If the request to /status was successful, even endpoints that are not explicitly set to null will not be assumed to be there, considering that the server should know best which endpoints exist. What do you think?

nichtich commented 3 years ago

This makes a lot of sense

stefandesu commented 3 years ago

I forgot to implement this because the issue was closed, oops. Will ship a 1.0.1 tomorrow.

stefandesu commented 3 years ago

[x] If the request to /status was successful, even endpoints that are not explicitly set to null will not be assumed to be there, considering that the server should know best which endpoints exist.
[x] Explain this in the documentation!