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

Add list of available locations #125

Closed
MetroMarv opened this issue Jun 30, 2020 · 7 comments · Fixed by #126
Closed

Add list of available locations #125

MetroMarv opened this issue Jun 30, 2020 · 7 comments · Fixed by #126

Comments

@MetroMarv
Copy link

In our case we need to retrieve a list of available locations (and their scheme) through the ICAR interface. In addition to scheme and id a description or name would be helpful.

This supports discoverability of supported schema/id combinations.

If necessary the list of schema/id combinations can be filtered by applying the user's privileges.

@MetroMarv
Copy link
Author

See the PR for a proposal of a simple locations endpoint.

@cookeac
Copy link
Collaborator

cookeac commented Jul 3, 2020

We discussed today:

  • rather than extend icarLocationIdentifierType, include an identifier field that is the primary identifier (of type icarLocationIdentifierType)
  • consider adding an array of alternativeIdentifiers, making clear in the description that these are for true 1:1 equivalents (so people don't confuse a farm and a business, for instance).

A note from me - the description for the location entity still references a Weight, so should be corrected.

@alamers
Copy link
Collaborator

alamers commented Jul 3, 2020

Consider authorization as well: this resource most likely only returns the list of identifiers that the client has access to. As we do not cover auth/autz yet, it may still be worth documenting.

Also consider the growth path: other, more advanced, use cases are:

  • locations that can provide a specific message type
  • exploring related identifiers to that location

As far as I can see, it seems that this url scheme does not necessarily block that but we should consider that, e.g.: /locations/{message-type} (or some other subresource) is ambiguous with /location/{scheme}, so we would then need to use /messages/{message-type}/locations for such a scenario (imho the better choice anyway)

In #109 there is also a request for other "meta data" such as available scheme's and available identifiers for a scheme. We should probably consider these here as well.

E.g.:

  • locations/{nl.kvk} is probably a valid query and fits within these scheme: returning a list of identifiers for this specific scheme

@MetroMarv
Copy link
Author

Sorry I had a day off yesterday, but now I pushed the discussed improvements to the PR:

  • use identifier property instead of inheritance
  • add alternative identifiers (including hint for the 1:1 mapping)
  • improve description of the \locations endpoint regarding the authorization
  • use Develop branch instead of ADE-1 branch for the PR base

@MetroMarv
Copy link
Author

@alamers I think we have to keep your points in mind, but I'd say there are no necessary changes for this resource for now.

@MetroMarv
Copy link
Author

Just one thing that came to my mind right now: To improve discoverability and ease of use of the API, maybe we can add a baseUrl field as well.

This could look like /locations/eu.farmId/276031231231234/ and avoids that API clients need to know how to construct that URL on their own. What do you think?

@cookeac
Copy link
Collaborator

cookeac commented Aug 17, 2020

Closing this now, but @MetroMarv if you would like to add the baseUrl field in ADE 1.1, please go ahead and re-open.

@cookeac cookeac closed this as completed Aug 17, 2020
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 a pull request may close this issue.

3 participants