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 database field to meta for describing the current database #424

Merged
merged 7 commits into from
Jun 17, 2023

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Sep 29, 2022

Closes #422.

This PR adds the optional database field to all meta responses. It can be used to provide the same data on an individual database as that found at the index meta-database, without requiring several requests and potential filtering of the index meta-database response.


Only other thought when writing this up is that this info could also be provided as a self entry in the links field for every response (though the fields would be limited to just using a meta field as part of a JSON API Links object).

If we prefer this route, then an alternative note could be added to the "JSON Response Schema: Common Fields" section of the spec that reserves the self keyword or something similar for this use, the only complication being that JSON:API self links should just return the exact URL of the current response.


Or one final option is just to add this kind of info at /info/ directly, and exclude it from meta.

optimade.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

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

Hi ml-evs,

I think it is good that we finally have a field to describe the database.
I still have a few small remarks but overall it looks good.

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

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

On line 655 there is still an example of a response. I think your database field should be added to this example.

@ml-evs
Copy link
Member Author

ml-evs commented Sep 30, 2022

On line 655 there is still an example of a response. I think your database field should be added to this example.

Not sure it's super important, as database is an optional field (and other optional fields are also omitted in that example, like implementation).

@JPBergsma
Copy link
Contributor

Yes you are right. As it is an optional field it is not so bad that it is not in the example on line 655.

@merkys
Copy link
Member

merkys commented Oct 3, 2022

Suggestion in #422 describes two places where this info can be added: /info endpoint (preferred then by @ml-evs) or meta of every response. I would also prefer /info to retain smaller responses, but maybe there are more arguments backing meta proposal?

Edit: I made up my mind, I support the current proposal now.

@JPBergsma
Copy link
Contributor

I agree with Merkys, that it would be sufficient to only place this info only at the info entry point. It is however a bit strange to return the data about the maintainer of the implementation with each response but not the data of the database maintainer. I think it would be best if both were moved to the info endpoint. This may however qualify as a breaking change, so perhaps we should do a major version update before implementing this.

@ml-evs ml-evs added this to the v1.2 milestone Dec 5, 2022
Copy link
Contributor

@rartino rartino left a comment

Choose a reason for hiding this comment

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

For parity with implementation, I think we should also allow to specify a version for the database from which the response comes.

I agree that it make sense to (also?) allow database information in /info, but I can see reasons why people may want it in every response from their database. It isn't many bytes.

optimade.rst Outdated Show resolved Hide resolved
@ml-evs
Copy link
Member Author

ml-evs commented Dec 31, 2022

For parity with implementation, I think we should also allow to specify a version for the database from which the response comes.

Fine by me.

I agree that it make sense to (also?) allow database information in /info, but I can see reasons why people may want it in every response from their database. It isn't many bytes.

I guess as it stands these fields could also be provided in the meta response from /info. If we are happy having them in meta then we should not duplicate it in /info too. I wouldn't be against moving it to just /info but perhaps that is a v2.0 change.

We could also consider adding a query parameter that elides the meta response, which could, for example, be included in pagination links from an implementation by default. ?meta=false or ?hide_meta etc. We have added a few unbounded fields to meta (e.g., description could be the full text of a paper describing the provider/database) so perhaps this would be helpful as an optional parameter.

I'll re-request review from everyone now to see if anyone has remaining objections to adding this to meta.

rartino
rartino previously approved these changes Jan 10, 2023
Copy link
Contributor

@rartino rartino 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 this - but I think the discussion shows that we have no policy with regards to what should go in the meta field of any response, and what belongs in /info. The lack of such policy is a problem, because it is going to make us - and our users - unsure where to find stuff.

JPBergsma
JPBergsma previously approved these changes Jan 19, 2023
Copy link
Member Author

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Reviewing my own PR here... I think we should also provide the database id in the meta of every response. This is another piece of data that is no provided anywhere but in the index meta-database. This can be used to construct globally unique IDs for entries within this database via {provider_prefix}/{database_id}/{entry_id (or immutable_id} (the provider prefix still needs a call to /info but at least its accessible from the same base URL).

One missing field other includes link aggregation, but that can probably be omitted here.

optimade.rst Show resolved Hide resolved
optimade.rst Show resolved Hide resolved
@merkys
Copy link
Member

merkys commented Jun 5, 2023

Regarding the placement, I noticed that "provider" is allowed to appear in "meta" of any response, thus it makes sense to me for "database" to follow suit (as is suggested here). Another point in favor of current proposal is provenance preservation - in case a database does not provide immutable IDs or URLs, this could be extracted from the response as a last-resort means. Therefore I am leaning to accepting (after merging develop in).

merkys
merkys previously approved these changes Jun 6, 2023
@merkys merkys requested review from JPBergsma and rartino June 6, 2023 06:40
@merkys merkys added the PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing label Jun 6, 2023
JPBergsma
JPBergsma previously approved these changes Jun 8, 2023
Copy link
Contributor

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

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

I do still have a preference for moving the description field to the info endpoint, as there is no reason to share it in every response.
As we have however already placed the description for the provider under meta in every response, it seems logical to do the same for the description field of the database.
So for now I will approve this change. But I do think that we should move it for the 2.0 release.

@ml-evs
Copy link
Member Author

ml-evs commented Jun 13, 2023

If we are happy with the field names etc here then I think @rartino this is ready for a final review+merge?

optimade.rst Outdated Show resolved Hide resolved
@ml-evs ml-evs dismissed stale reviews from JPBergsma and merkys via 6b9915e June 14, 2023 08:32
merkys
merkys previously approved these changes Jun 14, 2023
@merkys merkys requested a review from vaitkus June 14, 2023 10:12
optimade.rst Outdated Show resolved Hide resolved
ml-evs and others added 7 commits June 17, 2023 18:32
Co-authored-by: Johan Bergsma <29785380+JPBergsma@users.noreply.github.com>
Co-authored-by: Rickard Armiento <gitcommits@armiento.net>
Co-authored-by: Antanas Vaitkus <antanas.vaitkus90@gmail.com>
Co-authored-by: Rickard Armiento <gitcommits@armiento.net>
optimade.rst Show resolved Hide resolved
@ml-evs ml-evs merged commit 9f1381d into develop Jun 17, 2023
5 checks passed
@ml-evs ml-evs deleted the ml-evs/database_meta branch June 17, 2023 20:25
vaitkus added a commit to vaitkus/OPTIMADE that referenced this pull request Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add metadata field for describing the current database
5 participants