Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.
/ pulp Public archive

Problem: identity fields are serialized as full URLs #3607

Merged
merged 1 commit into from
Sep 4, 2018

Conversation

dkliban
Copy link
Member

@dkliban dkliban commented Aug 30, 2018

Solution: stop passing request to the serializer so it can only form relative URLs

This patch modifies the behavior of DetailIdentityField and it also adds an
additional

ItentityField
RelatedField
NestedRelatedField
NestedIdentityField

to replace

HyperlinkIdentityField
HyperlinkRelatedField
NestedHyperlinkedIdentityField
NestedHyperlinkedRelatedField

provided by DRF and DRF nested.

Required PR: pulp/pulp_file#114

re: #3850
https://pulp.plan.io/issues/3850

@pep8speaks
Copy link

pep8speaks commented Aug 30, 2018

Hello @dkliban! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on September 04, 2018 at 13:46 Hours UTC

@dkliban dkliban force-pushed the return-relative-href branch 5 times, most recently from bb392c0 to 0c999ec Compare August 31, 2018 19:24
@codecov
Copy link

codecov bot commented Aug 31, 2018

Codecov Report

Merging #3607 into master will decrease coverage by 0.06%.
The diff coverage is 71.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3607      +/-   ##
==========================================
- Coverage   56.79%   56.73%   -0.07%     
==========================================
  Files          60       60              
  Lines        2590     2607      +17     
==========================================
+ Hits         1471     1479       +8     
- Misses       1119     1128       +9
Impacted Files Coverage Δ
pulpcore/pulpcore/app/serializers/__init__.py 100% <ø> (ø) ⬆️
pulpcore/pulpcore/app/response.py 66.66% <0%> (ø) ⬆️
pulpcore/pulpcore/app/serializers/repository.py 94.44% <100%> (-0.04%) ⬇️
pulpcore/pulpcore/app/serializers/user.py 80% <100%> (ø) ⬆️
pulpcore/pulpcore/app/serializers/content.py 62.22% <100%> (ø) ⬆️
pulpcore/pulpcore/app/serializers/task.py 78.43% <100%> (ø) ⬆️
pulpcore/pulpcore/app/serializers/progress.py 100% <100%> (ø) ⬆️
pulpcore/pulpcore/app/serializers/fields.py 44.64% <33.33%> (ø) ⬆️
pulpcore/pulpcore/app/serializers/base.py 61.07% <50%> (-1.53%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57c5d4e...1099266. Read the comment docs.

dkliban added a commit to dkliban/pulp_file that referenced this pull request Aug 31, 2018
Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

While testing I see the worker url is still using a full URL. Here is a snipppet from the task status result:

"worker": "http://localhost:8000/pulp/api/v3/workers/2/",

@dkliban
Copy link
Member Author

dkliban commented Sep 2, 2018

I updated the task serializer.

Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

In the 'task' progress report I see a full url:

        {
            "message": "Downloading Metadata",
            "state": "completed",
            "total": 1,
            "done": 1,
            "suffix": "",
            "task": "http://localhost:8000/pulp/api/v3/tasks/00429392-009b-4a18-a3e8-20709a15f7ed/"
        },

Solution: stop passing request to the serializer so it can only form relative URLs

This patch modifies the behavior of DetailIdentityField and it also adds an
additional

ItentityField
RelatedField
NestedRelatedField
NestedIdentityField

to replace

HyperlinkIdentityField
HyperlinkRelatedField
NestedHyperlinkedIdentityField
NestedHyperlinkedRelatedField

provided by DRF and DRF nested.

Required PR: pulp/pulp_file#114

re: pulp#3850
https://pulp.plan.io/issues/3850
Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

@dkliban I tested this entirely just now also with PR 144 in pulp_file, and it all looks exactly correct. I tested it using your instructions. I also inspected all objects in the API looking for any stray full urls, but I found none.

Thank you for putting together this great work!

@dkliban dkliban merged commit d38aee4 into pulp:master Sep 4, 2018
daviddavis pushed a commit to daviddavis/pulp_file that referenced this pull request May 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants