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

Paging: Make ReadResource[] a ReadResourceSequence #169

Merged
merged 22 commits into from
May 7, 2020

Conversation

tobiasschweizer
Copy link
Contributor

@tobiasschweizer tobiasschweizer commented Mar 25, 2020

This PR makes ReadResource[] a ReadResourceSequence. This affects SearchEndpoint and ResourcesEndpoint.

See proposal https://github.com/dasch-swiss/knora-proposals/blob/master/designs/2020-03-23-knora-api-lib-js-paging.md

closes #161

} else {
return forkJoin([createReadResource(resourcesJsonld as { [index: string]: object[] | string }, ontologyCache, listNodeCache, jsonConvert)]);
// TODO: Is a check for knora-api:mayHaveMoreResults necessary?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benjamingeer I assume knora-api:mayHaveMoreResults is only present on a JSON-LD structure with a @graph. Is a response supporting paging always a @graph? What about a response where all results are removed but one? Would it still be a @graph?

Copy link
Contributor

Choose a reason for hiding this comment

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

You will get @graph only if one or both of the following is true:

  • The response contains more than one result.
  • The response includes knora-api:mayHaveMoreResults.

Here are examples.

A page containing one result with knora-api:mayHaveMoreResults:

{
  "@graph" : [ {
    "@id" : "http://rdfh.ch/0001/a-thing",
    "@type" : "anything:Thing",
    "knora-api:arkUrl" : {
      "@type" : "xsd:anyURI",
      "@value" : "http://0.0.0.0:3336/ark:/72163/1/0001/a=thingO"
    },
    "knora-api:attachedToProject" : {
      "@id" : "http://rdfh.ch/projects/0001"
    },
    "knora-api:attachedToUser" : {
      "@id" : "http://rdfh.ch/users/9XBCrDV3SRa7kS1WwynB4Q"
    },
    "knora-api:creationDate" : {
      "@type" : "xsd:dateTimeStamp",
      "@value" : "2016-03-02T15:05:10Z"
    },
    "knora-api:hasPermissions" : "CR knora-admin:Creator|M knora-admin:ProjectMember|V knora-admin:UnknownUser",
    "knora-api:userHasPermission" : "V",
    "knora-api:versionArkUrl" : {
      "@type" : "xsd:anyURI",
      "@value" : "http://0.0.0.0:3336/ark:/72163/1/0001/a=thingO.20160302T150510Z"
    },
    "rdfs:label" : "A thing"
  } ],
  "knora-api:mayHaveMoreResults" : true,
  "@context" : {
    "rdf" : "http://www.w3.org/1999/02/22-rdf-syntax-ns#",
    "knora-api" : "http://api.knora.org/ontology/knora-api/v2#",
    "rdfs" : "http://www.w3.org/2000/01/rdf-schema#",
    "xsd" : "http://www.w3.org/2001/XMLSchema#",
    "anything" : "http://0.0.0.0:3333/ontology/0001/anything/v2#"
  }
}

A page containing one result without knora-api:mayHaveMoreResults:

{
  "@id" : "http://rdfh.ch/0001/a-thing",
  "@type" : "anything:Thing",
  "knora-api:arkUrl" : {
    "@type" : "xsd:anyURI",
    "@value" : "http://0.0.0.0:3336/ark:/72163/1/0001/a=thingO"
  },
  "knora-api:attachedToProject" : {
    "@id" : "http://rdfh.ch/projects/0001"
  },
  "knora-api:attachedToUser" : {
    "@id" : "http://rdfh.ch/users/9XBCrDV3SRa7kS1WwynB4Q"
  },
  "knora-api:creationDate" : {
    "@type" : "xsd:dateTimeStamp",
    "@value" : "2016-03-02T15:05:10Z"
  },
  "knora-api:hasPermissions" : "CR knora-admin:Creator|M knora-admin:ProjectMember|V knora-admin:UnknownUser",
  "knora-api:userHasPermission" : "V",
  "knora-api:versionArkUrl" : {
    "@type" : "xsd:anyURI",
    "@value" : "http://0.0.0.0:3336/ark:/72163/1/0001/a=thingO.20160302T150510Z"
  },
  "rdfs:label" : "A thing",
  "@context" : {
    "rdf" : "http://www.w3.org/1999/02/22-rdf-syntax-ns#",
    "knora-api" : "http://api.knora.org/ontology/knora-api/v2#",
    "rdfs" : "http://www.w3.org/2000/01/rdf-schema#",
    "xsd" : "http://www.w3.org/2001/XMLSchema#",
    "anything" : "http://0.0.0.0:3333/ontology/0001/anything/v2#"
  }
}

This comment was marked as resolved.

This comment was marked as resolved.

Copy link
Contributor Author

@tobiasschweizer tobiasschweizer Mar 27, 2020

Choose a reason for hiding this comment

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

sorry, initially I misread your post. It's clear now :-)

If a user gets the first page of a 100 results and 24 of them are removed, the result is still represented as a @graph with the flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

but what if a user gets the first page of a 100 results and 24 of them are removed. Is the one result still represented as a @graph?

Yes, this would look like the first example above. If the response contains mayHaveMoreResults, you will always get a @graph.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this would look like the first example above. If the response contains mayHaveMoreResults, you will always get a @graph.

Yes, sorry, I just got that ;-) My bad

@tobiasschweizer
Copy link
Contributor Author

I also made ResourcesEndpoint.getResources return a ResourceSequence to make it consistent with SearchEndpoint. Does this make sense since ResourceEndpoint does not support paging?

@daschbot
Copy link
Collaborator

This pull request has been mentioned on Discuss DaSCH. There might be relevant details there:

https://discuss.dasch.swiss/t/knora-api-js-lib-pr-changing-public-api/158/1

@tobiasschweizer tobiasschweizer changed the title feature (paging): make ReadResource[] a ReadResourceSequence Paging: ReadResource[] a ReadResourceSequence Mar 25, 2020
@tobiasschweizer tobiasschweizer changed the title Paging: ReadResource[] a ReadResourceSequence Paging: Make ReadResource[] a ReadResourceSequence Mar 25, 2020
@tobiasschweizer
Copy link
Contributor Author

not to myself: remove change in makefile (checking out Knora branch) before merging

@benjamingeer
Copy link
Contributor

I also made ResourcesEndpoint.getResources return a ResourceSequence to make it consistent with SearchEndpoint. Does this make sense since ResourceEndpoint does not support paging?

Yes, I think it's fine, in that case mayHaveMoreResults will always be false.

@tobiasschweizer tobiasschweizer self-assigned this May 7, 2020
@tobiasschweizer
Copy link
Contributor Author

@benjamingeer Do you think this PR is good to go? I need an approval so I can merge it.

@benjamingeer
Copy link
Contributor

benjamingeer commented May 7, 2020

@tobiasschweizer Probably would make more sense to ask Flavie, Mike, or Sepideh to review front-end PRs.

@tobiasschweizer
Copy link
Contributor Author

Probably would make more sense to ask Flavie or Mike to review front-end PRs.

This PR deals with a change in serialisation hat will also have effects on the lib's public API. So I guess it makes sense to involve both backend and frontend developers.

I will deal with the changes in the public API in a UI lib PR that will then be reviewed by frontend developers.

Thanks for the review!

@benjamingeer
Copy link
Contributor

I guess it makes sense to involve both backend and frontend developers.

What I mean is that since I don't do any TypeScript programming myself and know very little TypeScript, it's hard for me to review TypeScript code. It would be better to ask someone who has more TypeScript programming experience.

@tobiasschweizer
Copy link
Contributor Author

What I mean is that since I don't do any TypeScript programming myself and know very little TypeScript, it's hard for me to review TypeScript code. It would be better to ask someone who has more TypeScript programming experience.

Yes, I understand this. But you are the only one that can answer questions like #169 (comment)

Maybe you should just be asked for questions specifically relating to serialisation in JSON-LD and someone else could review the actual implementation is TypeScript.

@tobiasschweizer tobiasschweizer merged commit 65b15ec into master May 7, 2020
@tobiasschweizer tobiasschweizer deleted the wip/161-paging-info branch May 7, 2020 13:37
@kilchenmann kilchenmann added the enhancement Improve existing code or new feature label Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing code or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Paging: Remove ForbiddenResource
4 participants