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] Wfs pagination in data fetcher #1126

Merged
merged 5 commits into from
Feb 21, 2025
Merged

[Datahub] Wfs pagination in data fetcher #1126

merged 5 commits into from
Feb 21, 2025

Conversation

AlitaBernachot
Copy link
Collaborator

@AlitaBernachot AlitaBernachot commented Feb 19, 2025

Description

This PR introduces pagination when reading data through a WFS service. This feature is implemented in a new reader: the WfsReader.

If the service supports pagination, the WfsReader will handle pagination, fetching api at each read(). If service version is lower than 2.0.0 (does not handle pagination) the WfsReader returns GeoJsonReader or GmlReader (works as previously).

Architectural changes

Added a new WfsReader.

Screenshots

None

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

Copy link
Contributor

github-actions bot commented Feb 19, 2025

Affected libs: feature-dataviz,
Affected apps: metadata-editor,

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

Copy link
Contributor

github-actions bot commented Feb 19, 2025

📷 Screenshots are here!

@AlitaBernachot AlitaBernachot force-pushed the wfs-datafetcher branch 8 times, most recently from ed85830 to 255637d Compare February 19, 2025 17:35
@AlitaBernachot AlitaBernachot marked this pull request as ready for review February 19, 2025 17:36
@AlitaBernachot AlitaBernachot requested a review from jahow February 19, 2025 17:36
Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

This looks really good! I'd like to test this first but I think your implementation makes a lot of sense, thanks!

Comment on lines 25 to 30
return this.getData().then(
(result) =>
({
itemsCount: result.items.length,
}) as DatasetInfo
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is supposed to be the total amount of items in the featureType, you should be able to get this information using ogc-client:

const count = (await this.endpoint.getFeatureTypeFull(featureTypeName)).objectCount

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right! i have updated the code like you have suggested.

Comment on lines 91 to 96
if (Array.isArray(this.sort) && this.sort.length > 0) {
const finalUrl = new URL(url)
const sorts = this.sort
.map(
(fieldSort) => `${fieldSort[1]}+${fieldSort[0] === 'asc' ? 'A' : 'D'}`
)
.join(',')

finalUrl.searchParams.append('sortBy', sorts)
url = finalUrl.toString()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

looking good! would be great to have this in ogc-client :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI i now set the params directly on the url string as searchParams is encoding the "+"

@AlitaBernachot AlitaBernachot force-pushed the wfs-datafetcher branch 3 times, most recently from 16b04f7 to 66a24b2 Compare February 20, 2025 09:00
@coveralls
Copy link

coveralls commented Feb 20, 2025

Coverage Status

coverage: 86.134% (+1.7%) from 84.386%
when pulling 445fa3f on wfs-datafetcher
into a73b34b on main.

Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

This looks great, thank you! I added a few comments for things that could be clarified/simplified. I tried this reader with #1120 and it seems to work OK.

Comment on lines 248 to 249
return this.getDownloadUrlsFromWfs(link.url.toString(), link.name).pipe(
switchMap((urls) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can probably just call openDataset right away:

Suggested change
return this.getDownloadUrlsFromWfs(link.url.toString(), link.name).pipe(
switchMap((urls) => {
return from(openDataset(wfsUrlEndpoint, 'wfs', {
wfsUrlEndpoint,
featureType: link.name,
})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, code updated

Comment on lines 34 to 35
url: string,
wfsUrlEndpoint: string,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand why you need both of these. Only using url everywhere would make it much simpler right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in the getDownloadUrlsFromWfs() the url Endpoint is proxified, this is why i have a wfsUrlEndpoint. Should i ignore the proxified url then? Agree it would be simpler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok i have removed one, now there is only one wfsUrlEndpoint

)
.join(',')

finalUrl.searchParams.append('sortBy', sorts)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
finalUrl.searchParams.append('sortBy', sorts)
finalUrl.searchParams.append('SORTBY', sorts)

to comply with the WFS spec :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated, thx

options?: {
namespace?: string
wfsVersion?: WfsVersion
wfsUrlEndpoint?: string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
wfsUrlEndpoint?: string
wfsFeatureType?: string

This would be more appropriate/explicit instead of reusing namespace

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok done, i kept namespace for gml and added wfsFeatureType for wfs, is that what you have in mind?

Copy link
Collaborator

Choose a reason for hiding this comment

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

exactly!

reader = await WfsReader.createReader(
url,
options.wfsUrlEndpoint,
options.namespace
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
options.namespace
options.wfsFeatureType

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -5,7 +5,7 @@ export function parseHeaders(httpHeaders: Headers): DatasetHeaders {
if (httpHeaders.has('Content-Type')) {
result.mimeType = httpHeaders.get('Content-Type').split(';')[0]
const supported =
SupportedTypes.filter(
SupportedTypes.filter((type) => type !== 'wfs').filter(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe add a comment here? just to make sure that someone encountering this code understands what's going on. Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@AlitaBernachot AlitaBernachot requested a review from jahow February 21, 2025 15:13
Copy link
Collaborator

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Looking good, thank you for the work!

@jahow
Copy link
Collaborator

jahow commented Feb 21, 2025

The e2e failures do not seem related, but that's a lot of failing tests :/

@jahow
Copy link
Collaborator

jahow commented Feb 21, 2025

Merging this PR as the failure is similar as the one on main

@jahow jahow merged commit 096cd6e into main Feb 21, 2025
13 of 14 checks passed
@jahow jahow deleted the wfs-datafetcher branch February 21, 2025 22:54
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