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.

Sign up for GitHub

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

Jump to bottom

Fix bad href substitution on pagination usage #14

Merged
merged 5 commits into from
May 30, 2023
Merged

Fix bad href substitution on pagination usage #14

merged 5 commits into from
May 30, 2023

Conversation

keul
Copy link
Contributor

@keul keul commented Apr 17, 2023 •
edited
Loading

This closes #13

Related Issue(s):

Description:

See #13

PR Checklist:

  • pre-commit hooks pass locally
  • Tests pass (run make test)
  • Documentation has been updated to reflect changes, if applicable, and docs build successfully (run make docs)
  • Changes are added to the CHANGELOG.

@gadomski gadomski self-requested a review April 17, 2023 19:06
@gadomski gadomski added this to the 2.4.6 milestone Apr 19, 2023
gadomski
gadomski requested changes Apr 19, 2023
View reviewed changes
Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

Change looks good to me, but can you add:

  • unit test
  • changelog entry

@gadomski gadomski self-requested a review April 21, 2023 19:49
@keul
Copy link
Contributor Author

keul commented Apr 28, 2023

I'll work on this ASAP!

@gadomski gadomski removed their request for review May 8, 2023 13:03
@keul keul force-pushed the fix-bad-href-with-token branch from 3222ead to 57f6502 Compare May 11, 2023 13:09
@keul keul marked this pull request as draft May 11, 2023 13:10
@keul
Copy link
Contributor Author

keul commented May 11, 2023

@gadomski done, let me know if the test is enough for you

@keul keul marked this pull request as ready for review May 11, 2023 13:57
@keul keul requested a review from gadomski May 11, 2023 13:57
gadomski
gadomski requested changes May 15, 2023
View reviewed changes
tests/resources/test_item.py Outdated
Comment on lines 702 to 705
resp_json = resp.json()
links = resp_json["links"]
assert links[0]["rel"] == "next"
assert links[0]["href"].startswith("http://testserver/search?")
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't rely on link ordering, we should find the link by rel. E.g.

Suggested change
resp_json = resp.json()
links = resp_json["links"]
assert links[0]["rel"] == "next"
assert links[0]["href"].startswith("http://testserver/search?")
resp_json = resp.json()
link = next(l for l in resp_json["links"] if l["rel"] == "next)
assert link["href"].startswith("http://testserver/search?")

tests/resources/test_item.py Outdated
Comment on lines 710 to 711
assert links[0]["rel"] == "next"
assert links[1]["rel"] == "previous"
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, we should not rely on link ordering, but instead fetch by rel.

@keul keul requested a review from gadomski May 26, 2023 07:57
gadomski
gadomski requested changes May 26, 2023
View reviewed changes
Copy link
Member

@gadomski gadomski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks like CI is failing due to some incorrect validation on the part of stac-pydantic -- bboxs are allowed to have a minx greater than the maxx if they cross the antimeridian. I started on a fix for the problem in stac-pydantic, but that repo is pretty out-of-date and has a lot of failing tests, so it wasn't going to be a quick fix.

I'm willing to merge this PR through that failing test (once my review comments are applied), but after that someone should go fix stac-pydantic. I'm not really funded to do so, so its unlikely that I'll be able to. stac-utils/stac-pydantic#122

tests/resources/test_item.py Outdated Show resolved Hide resolved
tests/resources/test_item.py Outdated Show resolved Hide resolved
keul and others added 2 commits May 26, 2023 16:54
@keul
Copy link
Contributor Author

keul commented May 26, 2023

Looks like CI is failing due to some incorrect validation on the part of stac-pydantic -- bboxs are allowed to have a minx greater than the maxx if they cross the antimeridian. I started on a fix for the problem in stac-pydantic, but that repo is pretty out-of-date and has a lot of failing tests, so it wasn't going to be a quick fix.

I'm willing to merge this PR through that failing test (once my review comments are applied), but after that someone should go fix stac-pydantic. I'm not really funded to do so, so its unlikely that I'll be able to. stac-utils/stac-pydantic#122

I can try to take a look there (I fear it will take some time, never digged into it)

gadomski
gadomski approved these changes May 30, 2023
View reviewed changes
Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

Approving and will merge even w/ CI failures because stac-pydantic needs a lot of work.

@gadomski gadomski merged commit ba7a199 into stac-utils:main May 30, 2023
@keul keul deleted the fix-bad-href-with-token branch May 30, 2023 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gadomski gadomski gadomski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.4.6
Development

Successfully merging this pull request may close these issues.

token pagination implementation is broken
2 participants
@keul @gadomski