Prevent request with page size of 9999 #3694

FrancescoMolinaro · 2024-11-26T09:47:59Z

References

Description

Correct page size parameter when provided as 9999.

Affected components:

EditBitstreamPageComponent - find All bitstream formats

is part of dynamic form rendered as DynamicSelectModel (ng-dynamic-forms documentation ) which does not support the infinite scroll or any other pagination feature

Solution:

Retrieve the bitstream formats using recursive pagination, fetching chunks of data with 20 elements per page until all data is retrieved. All data will be available in the dropdown, allowing the bitstream type to be selected.

item-bitstreams.component.ts

Solution:

Retrieved the first 10 bundles initially. If more pages are available, a "load more" button will appear. On button click, the bundles for the next page will be retrieved and appended.

scripts-select.component.ts

Solution:

Replaced the basic HTML select element with an ngbDropdown to enable the use of the infiniteScroll directive (ngx-infinite-scroll), as other examples in the application. This necessitated refactoring the component to align with the new template.

Load of the scripts will be on demand when the scroll reaches the end of the dropdown items.

Configuration: taking 20 elements per page

bundle-data.service.ts

Solution: Removed elementsPerPage: 9999

Elements are no longer statically defined in the final layer of the service; they are now passed as parameters from higher levels.

dynamic-list.component.ts

Solution: Figured all the values should be available all at once , so retrieved the data using recursive pagination, fetching chunks of data with 20 elements per page until all data is retrieved.

relationship-type-data.service.ts

Solution:

As instructed by the existing comment , values should be available all at once , so retrieved the data using recursive pagination, fetching chunks of data with 20 elements per page until all data is retrieved.

dynamic-lookup-relation-search-tab.component.ts

Solution:

selectAll method using the 9999 elements per page is not used , so it is removed.

submission-section-cc-licenses.component.ts

Solution:

Starting from the structure of the cc-license component, where the dropdown toggle is a separate component and options are rendered using content projection in another component, the solution would be using an RxJS operator to recursively retrieve all elements until the list is complete.

Additional point: other changes would require a more complex refactor of the component and the existing logic.

Configuration: taking 20 elements per page

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

…ize of 9999

…ge size of 9999

…croll instead of page size 9999

…ize of 9999

…g page size of 9999

…ion-search-tab.component)

…by removing page size of 9999

…e of 9999

github-actions · 2024-12-17T22:59:49Z

Hi @FrancescoMolinaro,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

…gular into task/main/DURACOM-304

artlowel

I don't agree with the approach of using expand to recursively retrieve all pages and still combine them as a single list on the client side. That doesn't solve the issue behind #2513.

The reason behind #2513 is that we shouldn't ever have a component on the client that requires all data from the server to work. The point of enforcing pagination is to keep the amount of data needed to start using a UI component to a minimum, both because it loads faster, and to reduce strain on the REST server. Getting all pages one at a time before we render something is arguably also slower and less efficient than just requesting everything in a single huge page.

What would solve #2513 imo is to replace all UI components that don't support working with one page at a time with ones that do. E.g. instead of a simple dropdown that contains all values, replace it with an "infinite scroll" style dropdown

I understand that that may be a lot of work for some of these components, and in other cases the reason why they used a huge page size in the first place was often that there was no rest endpoint to retrieve the exact information we need, so rest endpoints would likely need to be added first.

If you can't fix every instance in this PR, it's okay to leave some out, and leave #2513 open to be solved in later PRs

I wouldn't, however, replace one way of retrieving all data from the server and doing the processing client side, with another.

…opdown.component.ts

github-actions · 2025-01-14T16:43:21Z

Hi @FrancescoMolinaro,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

…n/DURACOM-304

…ic-list

FrancescoMolinaro · 2025-01-15T16:07:55Z

Hi @artlowel , thanks for your feedback!

I totally agree with your point and, following your suggestion, I have tried to improve the logic where possible, either using infinite scrolling or a "load more" button.

The only case where I had no luck to improve the solution is for:

src/app/core/data/relationship-type-data.service.ts

here unfortunately the call is made inside an effect and is not easily repleaceable, I think here we would need to add/extend an endpoint on the rest side as suggested also by a todo that was left for the method:

getRelationshipTypeByLabelAndTypes.

I will look forward to your feedback and thanks again for the review.

artlowel

Thanks @FrancescoMolinaro!

Most of it looks and works good now. I just have a few more inline comments

artlowel · 2025-01-16T14:46:05Z

src/app/item-page/edit-item-page/item-bitstreams/item-bitstreams.component.html

@@ -39,6 +39,9 @@
                                   [isFirstTable]="isFirst"
                                   aria-describedby="reorder-description">
    </ds-item-edit-bitstream-bundle>
+    <div class="d-flex justify-content-center" *ngIf="showLoadMoreLink$ | async">
+      <div class="btn btn-link py-3" (click)="loadBundles()"> {{'item.edit.bitstreams.load-more.link' | translate}}</div>


We've just done a major overhaul of this page to improve the accessibility in #3464

Please use an actual button here, so it can be accessed using the keyboard

artlowel · 2025-01-16T14:59:39Z

src/app/core/data/bundle-data.service.ts

+  findByItemAndName(item: Item, bundleName: string, useCachedVersionIfAvailable = true, reRequestOnStale = true, options?: FindListOptions, ...linksToFollow: FollowLinkConfig<Bundle>[]): Observable<RemoteData<Bundle>> {
+    return this.findAllByItem(item, options, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow).pipe(


I don't understand how this change will work. The method worked by retrieving all bundles and filtering on their name client side. If we no longer retrieve all bundles that filter won't work. The only way it can be fixed is with a rest endpoint I think

Hi @artlowel thanks for the catch!

I though we could have use the limitation for bitstreams in the config ( I believe is 5 per page ) but is actually not true, that changes case by case, my bad.

I have kept the implementation because I think the method should support anyway the pagination and instead of the previous loop with expand I set the pagination parameter of 9999, I know this goes in the opposite direction of the PR but I agree that, in terms of rendering time, one big call is preferable to n small calls.
The solution, as you mentioned, will have to be a new endpoint that returns all the bitstreams by bundle name.

In addition to this I have also adapted my changes to the dynamic-list.component.ts as I realized that a "load more" button was not the right approach.
The issue that I noticed was happening in case the user would select an option out of range, save the submission, then reload the page; in this case the user would have had to load all options before being able to see his own selection.

The new solution I adopted is to keep the paginated calls, render the first ones as soon as they are ready, then keep loading the next ones in the background, adding them to the view progressively.

I will look forward to your feedback and thanks again!

Hi @FrancescoMolinaro

I can't seem to find a submission config that actually ends up using dynamic-list.component.ts.

Before I tested it by simply replacing the tag component with the list component here, and checking the subject field. However that keeps loading infinitely for me now. That might be due to differences between tags and lists tough.

Could you please let me know the backend submission field config you're using to test it?

Hi @artlowel , to visualize a dynamic-list in the submission form I am using a configuration like the following:

<field> <dc-schema>dc</dc-schema> <dc-element>identifier</dc-element> <label>Identifiers</label> <input-type value-pairs-name="common_identifiers">list</input-type> <repeatable>true</repeatable> <required /> <hint>Test hint</hint> </field>

The needed part is an input type of value "list" and a vocabulary, then we have two possible list based on the "repeatable" parameter.
Let me know in case this shouldn't work.

Thanks, that config works.

I'd still remove the line break between the pages.

(In this screenshot I reduced the page size to 2, to get a better idea how pagination works)

After they've loaded it doesn't matter for the user where the page breaks were:

Hi @artlowel , thanks again for the feedback, I have removed the br, I agree it would just weirdly separate the options.

artlowel · 2025-01-16T15:00:16Z

src/app/core/data/bundle-data.service.ts

+  // findByItemAndName(item: Item, bundleName: string, useCachedVersionIfAvailable = true, reRequestOnStale = true, ...linksToFollow: FollowLinkConfig<Bundle>[]): Observable<RemoteData<Bundle>> {
+  //   return this.findAllByItem(item, { elementsPerPage: 1, currentPage: 1 }, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow).pipe(
+  //     expand((rd: RemoteData<PaginatedList<Bundle>>) => {
+  //       if (rd.hasSucceeded && hasValue(rd.payload) && hasValue(rd.payload.page) && rd.payload.currentPage < rd.payload.totalPages) {
+  //         const nextPageOptions = { elementsPerPage: 1, currentPage: rd.payload.currentPage + 1 };
+  //         return this.findAllByItem(item, nextPageOptions, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow);
+  //       } else {
+  //         return EMPTY;
+  //       }
+  //     }),
+  //     map((rd: RemoteData<PaginatedList<Bundle>>) => {
+  //       if (hasValue(rd.payload) && hasValue(rd.payload.page)) {
+  //         const matchingBundle = rd.payload.page.find((bundle: Bundle) => bundle.name === bundleName);
+  //         if (hasValue(matchingBundle)) {
+  //           return new RemoteData(
+  //             rd.timeCompleted,
+  //             rd.msToLive,
+  //             rd.lastUpdated,
+  //             RequestEntryState.Success,
+  //             null,
+  //             matchingBundle,
+  //             200
+  //           );
+  //         } else {
+  //           return new RemoteData(
+  //             rd.timeCompleted,
+  //             rd.msToLive,
+  //             rd.lastUpdated,
+  //             RequestEntryState.Error,
+  //             `The bundle with name ${bundleName} was not found.`,
+  //             null,
+  //             404
+  //           );
+  //         }
+  //       } else {
+  //         return rd as any;
+  //       }
+  //     })
+  //   );
+  // }


This can probably be removed

artlowel

Thanks @FrancescoMolinaro!

tdonohue

👍 Thanks @FrancescoMolinaro ! I tested this today, including testing all the modified components, and I couldn't find any obvious bugs. I also noticed (in some cases) where scrolling the dropdown loaded the next page, etc.

This appears to be working well to me. If any later issues are found, we can solve them in follow-up PRs.

dspace-bot · 2025-01-23T19:28:57Z

Backport failed for dspace-7_x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin dspace-7_x
git worktree add -d .worktree/backport-3694-to-dspace-7_x origin/dspace-7_x
cd .worktree/backport-3694-to-dspace-7_x
git switch --create backport-3694-to-dspace-7_x
git cherry-pick -x 369cb3eedfa30809e0a3ed60109832f00f2ee655 017d110da0d99e51ee3e66f222d58cbd3e1d5434 a9d151801b65088817496c02e7f1a9c811bec909 850a95172d382ed77b0d23d5e5f688864baeb2ec 4cd5e0aee170f749f451bc24496d3f55e0cbcc61 4830e4611211e21ceedc26b05f74c696c94f5b0b ebb408e492f21f529d7a678734a7b5856b359f41 4642ed1791e6a196a49bf423336f98a2e5860dae ac7af44c1dfb5387beabbbea83ff3a6628d151ec b43e1f4a8714cc596352d007fce5605561f00400 335fe621a003650e628b7a723d542c4a62d72889 d90a988e33a8181e3cac4549d58cef44d5dcaa61 f5ec5f9674fa3e404e08dbfa7eae7f7002445fa7 9f8083e9c539a76bdf45a49121ff5de4f4d9be1e 134ecd6d48167d3111f512abf997dfc739cb1c62 903daadcbf422f10d8c6af76c17e99c6bd7ad8e4 25b2f681ef341f70b17ab1feb0286e964c88ee9a c79affdec6a0a3f6b44bbd18be9ed72b2b95029a 9570cd9721891fc3fb3bef613e2758a3494f3dbf 2847dc39d9c8e9e23b5d353a6fb9ed1cb4e55798

dspace-bot · 2025-01-23T19:28:59Z

Backport failed for dspace-8_x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin dspace-8_x
git worktree add -d .worktree/backport-3694-to-dspace-8_x origin/dspace-8_x
cd .worktree/backport-3694-to-dspace-8_x
git switch --create backport-3694-to-dspace-8_x
git cherry-pick -x 369cb3eedfa30809e0a3ed60109832f00f2ee655 017d110da0d99e51ee3e66f222d58cbd3e1d5434 a9d151801b65088817496c02e7f1a9c811bec909 850a95172d382ed77b0d23d5e5f688864baeb2ec 4cd5e0aee170f749f451bc24496d3f55e0cbcc61 4830e4611211e21ceedc26b05f74c696c94f5b0b ebb408e492f21f529d7a678734a7b5856b359f41 4642ed1791e6a196a49bf423336f98a2e5860dae ac7af44c1dfb5387beabbbea83ff3a6628d151ec b43e1f4a8714cc596352d007fce5605561f00400 335fe621a003650e628b7a723d542c4a62d72889 d90a988e33a8181e3cac4549d58cef44d5dcaa61 f5ec5f9674fa3e404e08dbfa7eae7f7002445fa7 9f8083e9c539a76bdf45a49121ff5de4f4d9be1e 134ecd6d48167d3111f512abf997dfc739cb1c62 903daadcbf422f10d8c6af76c17e99c6bd7ad8e4 25b2f681ef341f70b17ab1feb0286e964c88ee9a c79affdec6a0a3f6b44bbd18be9ed72b2b95029a 9570cd9721891fc3fb3bef613e2758a3494f3dbf 2847dc39d9c8e9e23b5d353a6fb9ed1cb4e55798

* [DURACOM-304] Refactored item-bitstreams.component by removing page size of 9999 * [DURACOM-304] Refactored edit-bitstream-page.component by removing page size of 9999 * [DURACOM-304] Refactored scripts-select.component by using infinite scroll instead of page size 9999 * [DURACOM-304] Refactored dynamic-list.component.ts by removing page size of 9999 * [DURACOM-304] Refactored relationship-type-data.service.ts by removing page size of 9999 * [DURACOM-304] removed unneeded selectAll method (dynamic-lookup-relation-search-tab.component) * [DURACOM-304] Refactored submission-section-cc-licenses.component.ts by removing page size of 9999 * [DURACOM-304] lint fix * [DURACOM-304] test fix * [DURACOM-304] fix accessibility issue on scripts-select * [DURACOM-304] Refactor of bundle-data.service.ts by removing page size of 9999 * [DURACOM-304] other fix related to accessibility * [DURACOM-304] lint fix * [DURACOM-304] resolve conflicts * [DURACOM-304] fix lint * [DURACOM-304] add support for findAll method in dynamic-scrollable-dropdown.component.ts * [DURACOM-304] refactor to use lazy data provider * [DURACOM-304] improve loading logic for cc-licenses section and dynamic-list * [DURACOM-304] refactor, fix dynamic-list.component loading * [DURACOM-304] remove br --------- Co-authored-by: Alisa Ismailati <[email protected]>

tdonohue · 2025-01-23T19:48:38Z

@FrancescoMolinaro : I was able to easily port this to dspace-8_x in #3887. However, porting it to dspace-7_x appears to be much more complex (I'm seeing a large number of merge conflicts).

Would you be able to create a separate PR to port this to dspace-7_x? I'd gladly test it myself if you can create the port.

[Port dspace-8_x] Prevent request with page size of 9999 (#3694)

alisaismailati added 13 commits November 19, 2024 11:42

[DURACOM-304] Refactored item-bitstreams.component by removing page s…

369cb3e

…ize of 9999

[DURACOM-304] Refactored edit-bitstream-page.component by removing pa…

017d110

…ge size of 9999

[DURACOM-304] Refactored scripts-select.component by using infinite s…

a9d1518

…croll instead of page size 9999

[DURACOM-304] Refactored dynamic-list.component.ts by removing page s…

850a951

…ize of 9999

[DURACOM-304] Refactored relationship-type-data.service.ts by removin…

4cd5e0a

…g page size of 9999

[DURACOM-304] removed unneeded selectAll method (dynamic-lookup-relat…

4830e46

…ion-search-tab.component)

[DURACOM-304] Refactored submission-section-cc-licenses.component.ts …

ebb408e

…by removing page size of 9999

[DURACOM-304] lint fix

4642ed1

[DURACOM-304] test fix

ac7af44

[DURACOM-304] fix accessibility issue on scripts-select

b43e1f4

[DURACOM-304] Refactor of bundle-data.service.ts by removing page siz…

335fe62

…e of 9999

[DURACOM-304] other fix related to accessibility

d90a988

[DURACOM-304] lint fix

f5ec5f9

github-actions bot assigned FrancescoMolinaro Nov 26, 2024

tdonohue requested a review from artlowel December 12, 2024 15:42

github-actions bot added the merge conflict label Dec 17, 2024

FrancescoMolinaro added 3 commits December 18, 2024 13:37

Merge branch 'gitHub/main' into task/main/DURACOM-304

d4064f6

[DURACOM-304] resolve conflicts

9f8083e

Merge branch 'task/main/DURACOM-304' of github.com:4Science/dspace-an…

3ac6cb8

…gular into task/main/DURACOM-304

github-actions bot removed the merge conflict label Dec 18, 2024

[DURACOM-304] fix lint

134ecd6

artlowel requested changes Jan 9, 2025

View reviewed changes

[DURACOM-304] add support for findAll method in dynamic-scrollable-dr…

903daad

…opdown.component.ts

github-actions bot added the merge conflict label Jan 14, 2025

FrancescoMolinaro added 3 commits January 15, 2025 09:46

Merge branch 'main' of github.com:DSpace/dspace-angular into task/mai…

5f2cc86

…n/DURACOM-304

[DURACOM-304] refactor to use lazy data provider

25b2f68

[DURACOM-304] improve loading logic for cc-licenses section and dynam…

c79affd

…ic-list

github-actions bot removed the merge conflict label Jan 15, 2025

tdonohue requested review from artlowel and tdonohue January 16, 2025 14:42

artlowel requested changes Jan 16, 2025

View reviewed changes

[DURACOM-304] refactor, fix dynamic-list.component loading

9570cd9

tdonohue requested a review from artlowel January 21, 2025 21:14

[DURACOM-304] remove br

2847dc3

artlowel approved these changes Jan 23, 2025

View reviewed changes

tdonohue approved these changes Jan 23, 2025

View reviewed changes

tdonohue merged commit 33a091d into DSpace:main Jan 23, 2025
15 checks passed

tdonohue mentioned this pull request Jan 23, 2025

[Port dspace-8_x] Prevent request with page size of 9999 (#3694) #3887

Merged

tdonohue added a commit that referenced this pull request Jan 23, 2025

Merge pull request #3887 from tdonohue/port_3694_to_8x

2d7c0cc

[Port dspace-8_x] Prevent request with page size of 9999 (#3694)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent request with page size of 9999 #3694

Prevent request with page size of 9999 #3694

FrancescoMolinaro commented Nov 26, 2024 •

edited

Loading

github-actions bot commented Dec 17, 2024

artlowel left a comment

github-actions bot commented Jan 14, 2025

FrancescoMolinaro commented Jan 15, 2025

artlowel left a comment

artlowel Jan 16, 2025

artlowel Jan 16, 2025

FrancescoMolinaro Jan 17, 2025

artlowel Jan 21, 2025 •

edited

Loading

FrancescoMolinaro Jan 21, 2025

artlowel Jan 23, 2025

FrancescoMolinaro Jan 23, 2025

artlowel Jan 16, 2025

artlowel left a comment

tdonohue left a comment

dspace-bot commented Jan 23, 2025

dspace-bot commented Jan 23, 2025

tdonohue commented Jan 23, 2025

		findByItemAndName(item: Item, bundleName: string, useCachedVersionIfAvailable = true, reRequestOnStale = true, options?: FindListOptions, ...linksToFollow: FollowLinkConfig<Bundle>[]): Observable<RemoteData<Bundle>> {
		return this.findAllByItem(item, options, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow).pipe(

Prevent request with page size of 9999 #3694

Prevent request with page size of 9999 #3694

Conversation

FrancescoMolinaro commented Nov 26, 2024 • edited Loading

References

Description

Checklist

github-actions bot commented Dec 17, 2024

artlowel left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 14, 2025

FrancescoMolinaro commented Jan 15, 2025

artlowel left a comment

Choose a reason for hiding this comment

artlowel Jan 16, 2025

Choose a reason for hiding this comment

artlowel Jan 16, 2025

Choose a reason for hiding this comment

FrancescoMolinaro Jan 17, 2025

Choose a reason for hiding this comment

artlowel Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

FrancescoMolinaro Jan 21, 2025

Choose a reason for hiding this comment

artlowel Jan 23, 2025

Choose a reason for hiding this comment

FrancescoMolinaro Jan 23, 2025

Choose a reason for hiding this comment

artlowel Jan 16, 2025

Choose a reason for hiding this comment

artlowel left a comment

Choose a reason for hiding this comment

tdonohue left a comment

Choose a reason for hiding this comment

dspace-bot commented Jan 23, 2025

dspace-bot commented Jan 23, 2025

tdonohue commented Jan 23, 2025

FrancescoMolinaro commented Nov 26, 2024 •

edited

Loading

artlowel Jan 21, 2025 •

edited

Loading