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

Fix jsonData not passed correctly #203

Merged
merged 7 commits into from
Aug 22, 2024
Merged

Fix jsonData not passed correctly #203

merged 7 commits into from
Aug 22, 2024

Conversation

JR-1991
Copy link
Member

@JR-1991 JR-1991 commented Jul 19, 2024

Overview

This PR addresses the issue reported at datalad/datalad-dataverse#320, where metadata sent via NativeApi.replace_datafile wasn’t updating the file metadata correctly. The problem arose because the jsonData payload needs to be sent as form-data, which HTTPX’s json kwarg doesn’t handle properly.

The fix involves sending the payload via the data argument. However, since other endpoints require data to be sent through the json argument, this PR introduces a private method in api.py to determine the correct way to send the data. This method checks for the presence of the jsonData key and adjusts the argument accordingly. If the key isn’t found, the json argument is used by default.

To prevent similar issues in the future, the tests have been updated. The replace_datafile tests now ensure that the metadata is correctly updated.

TLDR

  • If jsonData key is present sent through data kwarg
  • If not, JSON payload is sent through json
  • Extended tests to verify metadata is updated correctly

@JR-1991 JR-1991 self-assigned this Jul 19, 2024
@JR-1991 JR-1991 added pkg:api api related activities prio:asap Fix as soon as possible labels Jul 19, 2024
@JR-1991 JR-1991 added this to the 0.3.4 milestone Jul 19, 2024
@mih
Copy link
Contributor

mih commented Jul 19, 2024

I can confirm that this change makes the previously failing test in datalad-dataverse pass again. Thank you!

@pdurbin
Copy link
Member

pdurbin commented Jul 19, 2024

Nice work, @JR-1991!

@shoeffner
Copy link
Collaborator

Please note @adswa's changes in datalad/datalad-dataverse#322 and my alternative fix in #207, which is a PR onto this branch, not the main branch.

Fix handling of empty metadata when uploading data files
@JR-1991
Copy link
Member Author

JR-1991 commented Jul 26, 2024

@shoeffner, thanks for the notice and the PR! Once I return from vacation, I will add a test to check for empty json_str to this PR.

@JR-1991 JR-1991 marked this pull request as ready for review July 30, 2024 08:46
@JR-1991
Copy link
Member Author

JR-1991 commented Jul 30, 2024

I've included a test case to verify whether the behavior is as expected when json_str is set to None in the case of uploading a file. Would either @shoeffner or @pdurbin be available to review the PR?

@pdurbin
Copy link
Member

pdurbin commented Jul 30, 2024

@shoeffner are you interested in "triage" access? I just sent you an invite. You should see it at https://github.com/gdcc 😄

Copy link
Collaborator

@shoeffner shoeffner left a comment

Choose a reason for hiding this comment

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

I haven't run the tests, so I trust you that it works :) The CI did, though, so this looks good!

This looks good and I just added a few comments as food for thought here and there. In general, I would merge it and then see what we can do in #189 to improve further.
Right now, this looks like a very good fix for the problem at hand.
Thanks!

:shipit:

tests/api/test_upload.py Show resolved Hide resolved
tests/api/test_upload.py Show resolved Hide resolved
tests/api/test_upload.py Outdated Show resolved Hide resolved
pyDataverse/api.py Show resolved Hide resolved
pyDataverse/api.py Show resolved Hide resolved
@JR-1991
Copy link
Member Author

JR-1991 commented Aug 21, 2024

@shoeffner I have updated this PR and specifically the tests to use pyDataverse methods instead of manual requests, as highlighted by you. The PR is now ready for review.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

I didn't test this but tests are passing (and there are lots of new tests!) and the code makes sense.

@JR-1991 JR-1991 merged commit e1b5d98 into main Aug 22, 2024
10 checks passed
@shoeffner
Copy link
Collaborator

Sorry for not responding earlier, but it looks good and I am happy it got merged!

shoeffner added a commit to shoeffner/pyDataverse that referenced this pull request Aug 24, 2024
This change was necessary due to rebasing the feature branch onto the json-data fixes in gdcc#203.

Relates to gdcc#201.
@shoeffner shoeffner mentioned this pull request Aug 24, 2024
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:api api related activities prio:asap Fix as soon as possible
Projects
Development

Successfully merging this pull request may close these issues.

4 participants