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

Create feedURLscheme.json #238

Merged
merged 2 commits into from
Aug 26, 2021
Merged

Conversation

cookeac
Copy link
Collaborator

@cookeac cookeac commented Aug 12, 2021

Create a URL scheme for feeds and feed management (storage, rations, intake). Support GET, batch and single POST.
Along with other PRs, resolves #154.

Create a URL scheme for feeds and feed management (storage, rations, intake). Support GET, batch and single POST.
Along with other commits, resolves adewg#154.
@cookeac cookeac requested review from ahokkonen, alamers and AndreasSchultzGEA and removed request for AndreasSchultzGEA August 18, 2021 05:34
Copy link
Contributor

@ahokkonen ahokkonen left a comment

Choose a reason for hiding this comment

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

Added same comment to batch url paths as in previous OR.

}
}
},
"/batches/{location-scheme}/{location-id}/feeds": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should "locations" path be added after /batches -> "/batches/locations/{location-scheme}/{location-id}/diagnoses"? This question also applies to all the other "batches" endpoints in feedURLScheme.json.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ahokkonen I felt this wasn't necessary. It makes sense for locations to be part of the URL when you are using REST semantics (GET, collection GET, single POST) because you are describing resources.
The batches semantics is really a form of RPC (here's a batch of stuff I'm sending you; do something useful with it and send back a specialised result message), so the /locations/ path could mislead about the way the message works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we had a thought somewhere that even for batches, they would be limited per location.
For us, having the location in the url means that we can assume that the batch concerns only that location and validate against that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do have the location filters in the URL so they can be limited per location.
However, given we are not really doing REST at this stage, the URL can have any set of strings in its fixed text that we would like. Are we saying we would like to imply location - and if so, is it locations/batches or batches/locations? It doesn't make a lot of sense to me when it is really just batch posting endpoints, but clearly we can do either.

Copy link
Contributor

Choose a reason for hiding this comment

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

For me it could work without "locations" -prefix/suffix all well, but as @alamers mentioned it is still batch per-location posting. I agree that this case is out off the REST specs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For symmetry reasons i'd suggest to keep the 'locations' suffix in; this is then similar to other endpoints. Also, it may open up other groupings (other than locations) that can be validated (although that probably is a bit theoretical...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. I think we could define the semantics for batches, but I have replaced /batches/ with /batches/locations/ in the URL scheme. I can do this in due course to the other PRs.

cookeac added a commit to cookeac/ICAR that referenced this pull request Aug 20, 2021
cookeac added a commit to cookeac/ICAR that referenced this pull request Aug 20, 2021
cookeac added a commit to cookeac/ICAR that referenced this pull request Aug 20, 2021
cookeac added a commit to cookeac/ICAR that referenced this pull request Aug 20, 2021
Replace /batches/ with /batches/locations as per comments in PR adewg#238
Copy link
Collaborator

@erwinspeybroeck erwinspeybroeck left a comment

Choose a reason for hiding this comment

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

ok for me, no remarks

@erwinspeybroeck erwinspeybroeck merged commit 77418bf into adewg:Develop Aug 26, 2021
@cookeac cookeac deleted the cookeac-feed branch August 27, 2021 06:11
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.

4 participants