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

Improvements to paginated TD retrieval spec #130

Closed
wants to merge 9 commits into from

Conversation

farshidtz
Copy link
Member

@farshidtz farshidtz commented Mar 6, 2021


Preview | Diff

"limit": {
"description": "The number of TDs in the collection",
"type": "number",
"default": 1
Copy link
Member Author

Choose a reason for hiding this comment

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

@sebastiankb @mmccool Is there a way to set default values in TD?
The URI Template does not support it after draft v5 and I couldn't find anything in the TD's DataSchema either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Best to create an issue about this in the TD repo, otherwise it may not get addressed.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for my late response. TD allows to use terms which are not defined in data schema yet. Information can be found here https://w3c.github.io/wot-thing-description/#sec-data-schema-vocabulary-definition

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I'll reply on the new issue: w3c/wot-thing-description#1093

@farshidtz farshidtz marked this pull request as ready for review March 9, 2021 16:31
@farshidtz farshidtz changed the title Specify listing API Improvements to paginated TD retrieval spec Mar 9, 2021
Copy link
Contributor

@AndreaCimminoArriaga AndreaCimminoArriaga left a comment

Choose a reason for hiding this comment

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

I'm approving the review, as only remark I would add the optional functionality to return in the header the pair "Link" and an ontology class URI (as specified in the LDPP). In this way when the TDs are returned by using the headers one could know the types of these TDs.

@farshidtz
Copy link
Member Author

farshidtz commented Mar 17, 2021

We could try to fully comply with RFC7233 Range Requests. This would let us query range of bytes (or range of TDs) by setting equivalent request headers.

To have dedicated resource URLs for the RESTful API, we could allow passing range request values in query. E.g.:

/td?range-start=0&range-end=99&range-unit=TDs gives the first 100 TDs
/td?range-start=0&range-end=99&range-unit=bytes gives the first 100 bytes

The above is inline with LDP Paging. Moreover, we could borrow LDPP's Links spec to have next link and etag to identify the state.

@AndreaCimminoArriaga
Copy link
Contributor

@farshidtz it makes sense to have both ranges, maybe we could even define others. I like the approach, it is sensible.

index.html Outdated
Response:
<pre>
HTTP/1.1 200 OK
Content-Range: TDs 10-12/*
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the range should be "10-11"
Otherwise it would be 3 more TDs and the count 12 does not match (0-11)

@farshidtz farshidtz marked this pull request as draft March 23, 2021 11:36
@farshidtz
Copy link
Member Author

I changed this to draft. As mentioned in my last comment, we should try to fully comply with one standard and define query arguments to make pages addressable by URL. But this makes the specification even longer and this is overkill when there are much simpler approaches; see #145.

@mmccool mmccool marked this pull request as ready for review March 29, 2021 22:19
@mmccool
Copy link
Contributor

mmccool commented Mar 29, 2021

Closing without merging, but as discussed Farshid will create a new PR with equivalent (but improved) content.

@farshidtz farshidtz mentioned this pull request Apr 13, 2021
3 tasks
@farshidtz farshidtz deleted the listing branch March 7, 2022 16: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.

None yet

5 participants