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

[Datahub] Allow disabled ogc-client cache per request #1147

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AlitaBernachot
Copy link
Collaborator

@AlitaBernachot AlitaBernachot commented Mar 5, 2025

Description

This PR allows disabling the ogc cache per request.

How to tests

This PR is a preparation work for the next PR where components will disable/enable cache according to dataset's update frequency when fetching data (eg. table and chart). So no manual testing.

Architectural changes

A new BaseCacheReader has been introduced from which all other readers (Wfs, Csv, Gml, Geojson, Excel) inherits the cacheActive private property.

Screenshots

No UI

Quality Assurance Checklist

  • Commit history is devoid of any merge commits and readable to facilitate reviews
  • If new logic ⚙️ is introduced: unit tests were added
  • If new user stories 🤏 are introduced: E2E tests were added
  • If new UI components 🕹️ are introduced: corresponding stories in Storybook were created
  • If breaking changes 🪚 are introduced: add the breaking change label
  • If bugs 🐞 are fixed: add the backport <release branch> label
  • The documentation website 📚 has received the love it deserves

@AlitaBernachot AlitaBernachot marked this pull request as ready for review March 5, 2025 17:53
Copy link
Contributor

github-actions bot commented Mar 5, 2025

Affected libs:
feature-dataviz,feature-record,feature-router,ui-dataviz,ui-search,feature-search,feature-map,util-data-fetcher

Affected apps:
metadata-editor,datahub,demo,webcomponents,map-viewer,search

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

@AlitaBernachot AlitaBernachot force-pushed the allow-disabling-cache-per-request branch from 593c2db to 3a8e928 Compare March 5, 2025 17:58
Copy link
Contributor

github-actions bot commented Mar 5, 2025

📷 Screenshots are here!

Copy link
Contributor

github-actions bot commented Mar 5, 2025

📷 Screenshots are here!

Copy link
Contributor

github-actions bot commented Mar 5, 2025

📷 Screenshots are here!

@AlitaBernachot AlitaBernachot force-pushed the allow-disabling-cache-per-request branch 3 times, most recently from bde5360 to 6da7bb0 Compare March 6, 2025 12:46
@AlitaBernachot AlitaBernachot force-pushed the allow-disabling-cache-per-request branch from 6da7bb0 to fda5a34 Compare March 6, 2025 13:01
Copy link
Collaborator

@cmoinier cmoinier left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! It looks good, just one suggestion but feel free to merge if you prefer to keep as is :)

url,
options.wfsFeatureType,
cacheActive
)
break
}
reader.load()
Copy link
Collaborator

@cmoinier cmoinier Mar 6, 2025

Choose a reason for hiding this comment

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

How about passing the cache param to load directly, instead of adding another Reader on top of the others (with base-cache) ? I find that the Readers are already hard to read...

I did it in my PR and it worked too : https://github.com/geonetwork/geonetwork-ui/pull/1149/files#diff-b68b461107422b0097c7a45b98e85da1f4fbbf2959a80521e3781ceeea07cb58

But that's just a suggestion, your way works fine too :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was to avoid changing functions signature. Ok I will simplify and add cache param like you suggest.

Copy link
Collaborator Author

@AlitaBernachot AlitaBernachot Mar 6, 2025

Choose a reason for hiding this comment

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

Ah I remember why I did this, it is because also i didn't want to pass the activeCache params through all the sub component, cause now i need to pass activeCache to data-view > table-view > data-table, whereas if the cacheActive option is in the Reader I would not need all this modifications. What do you think? maybe we can ask a third opinion? Which one is best:

  • let all the components and sub components to decide whether to cache or not (and then, we should add @Input activeCache to each components/child), simpler, but need edit everywhere when a request is done
  • or let the reader handle this one, less modifications, but sub components won't be allowed to deactivate cache (can only be done in parent that creates the reader), or well they can, but this would need to set the activeCache of the reader before requesting data.

Copy link
Collaborator

@cmoinier cmoinier Mar 7, 2025

Choose a reason for hiding this comment

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

Mmmh okay, explained like that your way makes actually more sense than mine! I think that no matter which component (data/table etc), the link will always have the same update frequency anyway, so it will be cached in all cases --> and that's what this ticket is all about.

@coveralls
Copy link

Coverage Status

coverage: 85.714% (+2.8%) from 82.949%
when pulling 6da7bb0 on allow-disabling-cache-per-request
into da7bc31 on main.

@coveralls
Copy link

Coverage Status

coverage: 85.714% (+4.4%) from 81.342%
when pulling fda5a34 on allow-disabling-cache-per-request
into a16ba66 on main.

@AlitaBernachot AlitaBernachot force-pushed the allow-disabling-cache-per-request branch from 03b3324 to 0513459 Compare March 6, 2025 18:07
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