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

added json-ld contexts to api response #95

Merged
merged 7 commits into from
Apr 10, 2024
Merged

added json-ld contexts to api response #95

merged 7 commits into from
Apr 10, 2024

Conversation

sherwoodf
Copy link
Contributor

@sherwoodf sherwoodf commented Apr 5, 2024

After this change requesting any of the following objects:

  • Biosample
  • Collection
  • ImageAcquisition
  • Image
  • Specimen
  • Study
  • StudyFileReference
    will return json with an @context field with a URL that points to a file in this github repo. The content type will still be 'application/json', regardless of what gets passed in.

List of changes:

  1. Created JSON LD context objects, with some mapping to defined concepts in external ontologies where relevant. Includes ID mappings for objects that should point back to the appropriate BIA api endpoints. Included a generic vocabulary for all undefined terms.

  2. Updated models to automatically add in a context object with a link to the github repository path where these files will exist

  3. Added test that attempts to parse the json-ld using the contexts, in a similar manner to how i would expect users to parse the json. There is a slight difference, in that they should not need to manually insert the jsonld context (the link should just work), but you can't test changes while pointing to the link.

  4. changed tests that rely on the models to have some placeholder value for the context

  5. changed the imports in the tests to avoid from X import *

  6. Added some type checking & comment to the util method i created for checking unordered lists of dicts were equivalent.

  7. updated the env template with a few extra pointers on how to set up testing

@sherwoodf sherwoodf requested review from liviuba and ctr26 April 5, 2024 16:22
@ctr26
Copy link
Contributor

ctr26 commented Apr 8, 2024

This is an unfinished thought but I think I should note here anyway.

I'd prefer if the contexts string field(s) were constructed by a class. Either through composition or mixin, though I think my preference so far would be a composition context class that has a repr that returns a string. A mixin would need a post_init function which might clash with other future inheritors.

My reason being repeating the same string multiple times is error prone in future development where a developer might change one field correctly but miss another not another. Moreover there might be cases where you want the url to change (maybe to a file:/// for local development or something) and then that should be changed in the .env file ( eventually not today). It also has the advantage of future version bumps being smaller PRs.

Also, using the raw. GitHub path is better than the tree one. For pure file serving a common pattern I've seen is using the GitHub.io path. This is what is done for helm file serving, but in this case I don't see any meaningful advantage.

Thoughts? If be interested in knowing if my above points are moot because of the nature of the context.

@sherwoodf
Copy link
Contributor Author

sherwoodf commented Apr 8, 2024

I'd prefer if the contexts string field(s) were constructed by a class. Either through composition or mixin, though I think my preference so far would be a composition context class that has a repr that returns a string. A mixin would need a post_init function which might clash with other future inheritors.

My reason being repeating the same string multiple times is error prone in future development where a developer might change one field correctly but miss another not another. Moreover there might be cases where you want the url to change (maybe to a file:/// for local development or something) and then that should be changed in the .env file ( eventually not today). It also has the advantage of future version bumps being smaller PRs.

Thoughts? If be interested in knowing if my above points are moot because of the nature of the context.

Definitely not moot. I had initially create a base dictionary which had some of the content used in all contexts (vocab, title) etc which was then used in the more specific ones which i used to create the json directly in objects. When i switched to the json file i removed that 'hierarchy'.

While I agree that the current system would be prone to errors with changes, i don't think making a better one now is worth development time. It feels too early to try to create the 'correct' system as i'm not confident that we know what result we're aiming for. All the values should currently be conisdered placeholders: most of the important ones don't have an existing definitions, and where i have created a mapping, i'd bet against changes in one file needing to be reflected in all the others. I'd feel more comfortable coming back to it in ~4 months time to create a system that reflects the data-modelling decisions of the working group.

I view this more change as getting the api to return valid JSON-LD to make valid RDF to play with, but no where near the 'correct' RDF that conforms to an ontology of our api concepts (which still needs defining).

@ctr26
Copy link
Contributor

ctr26 commented Apr 8, 2024

Yep, valid reasons.

If the structure is being actively developed and is prone to significant change anyway then paying some technical debt now is your decision.

Thanks for the clear and thoughtful response

@sherwoodf sherwoodf requested a review from liviuba April 10, 2024 09:01
Copy link
Contributor

@liviuba liviuba left a comment

Choose a reason for hiding this comment

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

Looks good, please check suggestions

api/src/models/persistence.py Outdated Show resolved Hide resolved
api/src/tests/util.py Outdated Show resolved Hide resolved
@sherwoodf sherwoodf requested a review from liviuba April 10, 2024 10:00
Copy link
Contributor

@liviuba liviuba left a comment

Choose a reason for hiding this comment

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

LGTM!

@sherwoodf sherwoodf removed the request for review from ctr26 April 10, 2024 14:55
@sherwoodf sherwoodf merged commit 39b06fb into main Apr 10, 2024
0 of 7 checks passed
@sherwoodf sherwoodf deleted the jsonld branch July 4, 2024 15:20
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.

3 participants