Skip to content

Ensure yarn test:rest reads all JSON data, not just first chunk#1820

Merged
tdonohue merged 1 commit intoDSpace:mainfrom
tdonohue:fix_test_to_read_all_data
Sep 15, 2022
Merged

Ensure yarn test:rest reads all JSON data, not just first chunk#1820
tdonohue merged 1 commit intoDSpace:mainfrom
tdonohue:fix_test_to_read_all_data

Conversation

@tdonohue
Copy link
Copy Markdown
Member

@tdonohue tdonohue commented Sep 12, 2022

References

Fixes issue reported in https://groups.google.com/g/dspace-tech/c/XU7KFZGOr94/m/aeAlQZo5BQAJ

Description

Some proxies (namely nginx) may chunk JSON data in responses from the REST API. This tiny PR ensures that the yarn test:rest script can handle chunked data.

Simply put, it updates the existing code to use the 'data' and 'end' events to ensure that we don't just read the first chunk of data, and instead keep reading until the 'end' event fires. See the docs on http.ClientRequest for more details: https://nodejs.org/api/http.html#class-httpclientrequest

Instructions for Reviewers

  • Review the changes
  • Test yarn test:rest on a valid REST API and ensure it works. If you happen to have a REST API where data is returned in chunks that'd be even better.

This PR is based heavily on code sent to me by Sean Kalynuk (U of Manitoba) (@uofmsean) who reported to have this fix in this thread: https://groups.google.com/g/dspace-tech/c/XU7KFZGOr94/m/yZPYL6xBCgAJ

@tdonohue tdonohue added bug configuration 1 APPROVAL pull request only requires a single approval to merge labels Sep 12, 2022
@tdonohue tdonohue added this to the 7.4 milestone Sep 12, 2022
@tdonohue tdonohue self-assigned this Sep 12, 2022
@uofmsean
Copy link
Copy Markdown
Contributor

Confirmed that this fixes the issue when testing with the REST API via http and https. Tested against both an nginx reverse proxy (http) and also our F5 load balancer (https). Ensured that both were returning chunked data while testing.

@tdonohue
Copy link
Copy Markdown
Member Author

As agreed in today's Dev Meeting, I'm merging this immediately as the code was initially provided by @uofmsean ... and it has been tested by both myself and @uofmsean .

@tdonohue tdonohue merged commit 3665071 into DSpace:main Sep 15, 2022
@tdonohue tdonohue deleted the fix_test_to_read_all_data branch September 15, 2022 18:30
@byteAr
Copy link
Copy Markdown

byteAr commented May 10, 2023

Hello, what should I do exactly to solve this problem? I know maybe I should do a merge. I have Dspace 7.3 installed on my Debian 11 server. Greetings from Argentina

@tdonohue
Copy link
Copy Markdown
Member Author

@byteAr : This fix was added in 7.4. The easiest way to apply this fix is to simply upgrade to 7.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge bug configuration

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants