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

Dynamic datalink #212

Merged
merged 6 commits into from
Sep 29, 2023
Merged

Dynamic datalink #212

merged 6 commits into from
Sep 29, 2023

Conversation

agy-why
Copy link
Member

@agy-why agy-why commented Sep 1, 2023

This is the first attempt to create dynamic datalink.

Why dynamic datalink?

making use of tables for datalink is useful since it allows more flexibility (for non-programmable entries) and it is easier to maintain.

Additionally the datalink entries are usually part of the release, and as such get a DOI.

However, in one cases making use of tables to provide datalink entries may not be confortable:

  • in case of too many entries: like in Gaia, several datalink entries per source_id (same for applause...)

In these cases datalink entries generated on the fly based on the ID are necessary.

Implementation

I added a DynamicDatalinkAdapterMixin class that take care of the dynamic datalink entries. I also added a method called: get_datalink_rows and get_dyn_datalink_links to the BaseDatalinkAdapter, to provide a single point of gathering for the datalink entries (slighlty slower).

This improve greatly the maintenance and flexibility.

These changes should not break the former functionality.

How to

With this new implementation it is possible to add new datalink entries by overwriting the method:

get_dyn_datalink_links from the DatalinkAdapter. This method returns a list of dynamic datalink entries for the provided IDs. The user is free of implementing anything necessary. It just need to declare the new DatalinkAdapter in settings/base.py

Improvements

This pull request also solves some issues the previous implementation had:

  1. There were various place in the code where the datalink data were gathered, making maintenance difficult
  2. There were no flexibility in the way OAI and Datalink were connected (now there is an adapter).

@agy-why
Copy link
Member Author

agy-why commented Sep 1, 2023

This is working with the dynamic-datalink branch of gaia-app

@agy-why agy-why self-assigned this Sep 1, 2023
@agy-why agy-why added app:oai concern the oai-app priority:high app:datalink concern the datalink-app category:datalink feature feature request labels Sep 1, 2023
@agy-why agy-why added this to the MiniRelease milestone Sep 1, 2023
rows = list(Datalink.objects.filter(ID__in=identifiers).values_list(*field_names))

# get the dynamic datalink entries
dyn_rows = [tuple(link.values()) for link in self.get_dyn_datalink_links(identifiers)]
Copy link
Member Author

Choose a reason for hiding this comment

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

tuple(link.values()) does not guaranty the order of the items in the resulting array.

link['description'],
link['semantics'],
link['content_type'],
link['content_size']) for link in self.get_dyn_datalink_links(identifiers)
Copy link
Contributor

Choose a reason for hiding this comment

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

content_size should be changed to content_length (datalink standard)

Copy link
Contributor

@kimakan kimakan left a comment

Choose a reason for hiding this comment

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

The current implementation breaks rebuild_oai_schema. The culprit is the re-implemented get_datalink_list which is used by rebuild_oai_schema. In general, this function must return all datalink entries with the semantic #doi. This must be addressed.

daiquiri/oai/adapter.py Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Sep 26, 2023

Pull Request Test Coverage Report for Build 6311071061

  • 9 of 40 (22.5%) changed or added relevant lines in 5 files are covered.
  • 4 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.1%) to 63.98%

Changes Missing Coverage Covered Lines Changed/Added Lines %
daiquiri/datalink/viewsets.py 1 3 33.33%
daiquiri/oai/adapter.py 1 4 25.0%
daiquiri/query/models.py 0 5 0.0%
daiquiri/datalink/adapter.py 6 27 22.22%
Files with Coverage Reduction New Missed Lines %
daiquiri/datalink/viewsets.py 1 47.83%
daiquiri/query/models.py 1 71.29%
daiquiri/datalink/adapter.py 2 41.13%
Totals Coverage Status
Change from base Build 5715994248: -0.1%
Covered Lines: 4723
Relevant Lines: 7382

💛 - Coveralls

@kimakan kimakan merged commit 0f0442d into master Sep 29, 2023
10 checks passed
@kimakan kimakan deleted the dynamic-datalink branch October 23, 2023 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app:datalink concern the datalink-app app:oai concern the oai-app category:datalink feature feature request priority:high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants