Page MenuHomeSoftware Heritage

storage*: origin_visit_get(...) -> PagedResult[OriginVisit]
ClosedPublic

Authored by ardumont on Mon, Jul 27, 10:10 PM.

Details

Summary

Possible improvments for the allowed order (asc, desc) which could be an enum exposed in interface.py (D3629)

Impacts only swh-deposit [1] and swh-web [2]:

grep -r 'origin_visit_get(' ../*/swh/** | grep -v "swh-storage"
../swh-deposit/swh/deposit/migrations/0018_migrate_swhids.py:    all_visits = storage.origin_visit_get(origin)
../swh-web/swh/web/common/service.py:    for visit in storage.origin_visit_get(
../swh-web/swh/web/tests/browse/views/test_origin.py:    visits = archive_data.origin_visit_get(origin["url"])
../swh-web/swh/web/tests/browse/views/test_origin.py:    origin_visits = archive_data.origin_visit_get(origin["url"])
../swh-web/swh/web/tests/browse/views/test_origin.py:    origin_visits = archive_data.origin_visit_get(origin["url"])
../swh-web/swh/web/tests/browse/views/test_origin.py:    origin_visits = archive_data.origin_visit_get(origin["url"])
../swh-web/swh/web/tests/browse/views/test_origin.py:    origin_visits = archive_data.origin_visit_get(origin["url"])
../swh-web/swh/web/tests/browse/views/test_origin.py:    origin_visits = archive_data.origin_visit_get(origin["url"])
../swh-web/swh/web/tests/browse/views/test_content.py:    visits = archive_data.origin_visit_get(origin["url"])
../swh-web/swh/web/tests/browse/views/test_content.py:    visits = archive_data.origin_visit_get(origin["url"])
../swh-web/swh/web/tests/browse/views/test_directory.py:    visits = archive_data.origin_visit_get(origin["url"])
../swh-web/swh/web/tests/browse/views/test_directory.py:    visits = archive_data.origin_visit_get(origin["url"])
../swh-web/swh/web/tests/browse/test_snapshot_context.py:    visits = archive_data.origin_visit_get(origin["url"])
../swh-web/swh/web/tests/common/test_identifiers.py:    visits = archive_data.origin_visit_get(origin["url"])
../swh-web/swh/web/tests/common/test_identifiers.py:    visits = archive_data.origin_visit_get(origin["url"])
../swh-web/swh/web/tests/common/test_service.py:    expected_visits = archive_data.origin_visit_get(new_origin.url)
../swh-web/swh/web/tests/common/test_service.py:    origin_visit = archive_data.origin_visit_get(origin["url"])[-1]
../swh-web/swh/web/tests/conftest.py:    def origin_visit_get(self, origin_url):
../swh-web/swh/web/tests/conftest.py:        visits = list(self.storage.origin_visit_get(origin_url))
../swh-web/swh/web/tests/strategies.py:        visits = list(tests_data["storage"].origin_visit_get(origin["url"]))

[1] D3645

[2] D3647

Related to T645

Test Plan

tox

Diff Detail

Repository
rDSTO Storage manager
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
vlorentz added inline comments.Tue, Jul 28, 2:03 PM
swh/storage/api/serializers.py
34 ↗(On Diff #12774)

make it check __type__is "PagedResult"

50 ↗(On Diff #12774)

"storage" instead of "model_paged_result"

60 ↗(On Diff #12774)

same

Actually, now that I think of it... I think this PagedResult structure should go in swh.core.api. It would also be used by swh-search, and potentially by swh-index-storage.

vlorentz requested changes to this revision.Tue, Jul 28, 2:04 PM
This revision now requires changes to proceed.Tue, Jul 28, 2:04 PM
vlorentz added inline comments.
swh/storage/api/serializers.py
50 ↗(On Diff #12774)

or "storage_class". I can't find a satisfying name.

Actually, now that I think of it... I think this PagedResult structure should go in swh.core.api. It would also be used by swh-search, and potentially by swh-index-storage.

Are we sure that this will be used in those other modules?

Pretty much, yes. swh-search already has pagination very close to swh-storage

ardumont added inline comments.Tue, Jul 28, 2:19 PM
swh/storage/api/serializers.py
50 ↗(On Diff #12774)

(I saw that when doing D3629 but waited on you to make a remark ;)

well that's storage_model really.
It's the original model one which may be too broad.

Pretty much, yes. swh-search already has pagination very close to swh-storage

Ok, moving PagedResult up a notch in core then.

Pretty much, yes. swh-search already has pagination very close to swh-storage

Ok, moving PagedResult up a notch in core then.

Wait, why not in swh.model again?

Because it's not part of the data model, it's just used for RPC

ardumont added inline comments.Tue, Jul 28, 3:00 PM
swh/storage/api/serializers.py
29 ↗(On Diff #12774)

turns out that can be simplified to

"results": obj.results,

the remaining part of the serialization will do the rest.

36 ↗(On Diff #12774)

turns out that can be simplified to

results=obj["results"]

Actually, now that I think of it... I think this PagedResult structure should go in swh.core.api. It would also be used by swh-search, and potentially by swh-index-storage.

See D3632 ;)

ardumont updated this revision to Diff 12779.Tue, Jul 28, 3:54 PM
  • Refactor using D3632 code (PagedResult moved in core)

Note:

  • tox will fail until we land and release the new core
  • locally everything is fine

Build has FAILED

Patch application report for D3627 (id=12779)

Rebasing onto 77960ca4b5...

Current branch diff-target is up to date.
Changes applied before test
commit 3df99d4ea1c450bfb8bd6394b3182fb05d7521ca
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jul 27 22:17:59 2020 +0200

    storage*: origin_visit_get(...) -> PagedResult[OriginVisit]
    
    Related to T645

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/583/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/583/console

ardumont edited the test plan for this revision. (Show Details)Wed, Jul 29, 7:57 AM
ardumont updated this revision to Diff 12792.EditedWed, Jul 29, 8:00 AM

Use swh.core PagedResult and alias a storage with token as str

heads up, tox mypy, tests (local) and me are happy with this. I just open a
specific type for the token to be a string to follow the current dev on this
diff.

pre-commit mypy is not ¯\_(ツ)_/¯ though [1]
I don't get the pre-commit mypy hints yet.

Note: this diff is now 2 commits for now as i'm not yet sure of the alias
things (thank you for that pre-commit mypy...)

[1] https://forge.softwareheritage.org/P731

Build has FAILED

Patch application report for D3627 (id=12792)

Rebasing onto 77960ca4b5...

Current branch diff-target is up to date.
Changes applied before test
commit 27618b59d766e3e5a65acdace6424b15549490af
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Wed Jul 29 07:41:21 2020 +0200

    Use swh.core PagedResult and alias a storage with token as str
    
    It makes sense to me but pre-commit mypy is unhappy with this and I don't get
    its hints.
    
    Related to T645

commit 3df99d4ea1c450bfb8bd6394b3182fb05d7521ca
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jul 27 22:17:59 2020 +0200

    storage*: origin_visit_get(...) -> PagedResult[OriginVisit]
    
    Related to T645

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/585/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/585/console

You can remove all your changed from serializers.py now

vlorentz added inline comments.Wed, Jul 29, 9:11 AM
swh/storage/interface.py
37

PagedResult is not an instance of CorePagedResult[TResult, str], it's CorePagedResult[TResult, str] itself. Therefore:

PagedResult: Type[CorePagedResult[TResult, str]] = CorePagedResult[TResult, str]
41–42

if you want this one to work, don't reuse the same TypeVar:

TResult = TypeVar("TResult")

class PagedResult(CorePagedResult[TResult, str]):
    pass
vlorentz requested changes to this revision.Wed, Jul 29, 9:15 AM
This revision now requires changes to proceed.Wed, Jul 29, 9:15 AM

You can remove all your changed from serializers.py now

Yeah, i'm gonna add those for another diff.

swh/storage/interface.py
37

I tried that as well...

swh/storage/interface.py:36: error: Type variable "swh.core.api.classes.TResult" is unbound
swh/storage/interface.py:36: note: (Hint: Use "Generic[TResult]" or "Protocol[TResult]" base class to bind "TResult" inside a class)
swh/storage/interface.py:36: note: (Hint: Use "TResult" in function signature to bind "TResult" inside a function)
swh/storage/interface.py:810: error: Variable "swh.storage.interface.PagedResult" is not valid as a type
swh/storage/interface.py:810: note: See https://mypy.readthedocs.io/en/latest/common_issues.html#variables-vs-type-aliases
swh/storage/in_memory.py:869: error: Variable "swh.storage.interface.PagedResult" is not valid as a type
swh/storage/in_memory.py:869: note: See https://mypy.readthedocs.io/en/latest/common_issues.html#variables-vs-type-aliases
swh/storage/cassandra/storage.py:851: error: Variable "swh.storage.interface.PagedResult" is not valid as a type
swh/storage/cassandra/storage.py:851: note: See https://mypy.readthedocs.io/en/latest/common_issues.html#variables-vs-type-aliases
swh/storage/storage.py:890: error: Variable "swh.storage.interface.PagedResult" is not valid as a type
swh/storage/storage.py:890: note: See https://mypy.readthedocs.io/en/latest/common_issues.html#variables-vs-type-aliases
Found 5 errors in 4 files (checked 68 source files)
41–42

if you want this one to work, don't reuse the same TypeVar:

why?


I recall having tested that (without redefining TResult though) that would not work...
I'll try again, thanks.

vlorentz added inline comments.Wed, Jul 29, 10:37 AM
swh/storage/interface.py
37

ah yes, that makes sense.

41–42

I'm guessing it makes mypy glitchy to use the same typevar in both a generic class in a subclass of that class

ardumont updated this revision to Diff 12798.Wed, Jul 29, 10:41 AM
  • Adapt part of the review
  • tests are failing the comparison [1] for some unknown reason yet (i exploded the comparison for now)

(still need to cleanup the serialization part)

[1] P732

Build has FAILED

Patch application report for D3627 (id=12798)

Rebasing onto 77960ca4b5...

Current branch diff-target is up to date.
Changes applied before test
commit 28d930d4e6676a6040d53e47c74bec807418e6d2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jul 27 22:17:59 2020 +0200

    storage*: origin_visit_get(...) -> PagedResult[OriginVisit]
    
    Related to T645

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/588/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/588/console

ardumont updated this revision to Diff 12802.Wed, Jul 29, 11:23 AM

Fix swh.storage.interface.PagedResult type so everyone is happy (all mypys,
tests, flake...)

ardumont marked 4 inline comments as done.Wed, Jul 29, 11:30 AM
ardumont updated this revision to Diff 12803.Wed, Jul 29, 11:30 AM

Drop the serialization changes as it's irrelevant to the diff now

Build is green

Patch application report for D3627 (id=12802)

Rebasing onto 77960ca4b5...

Current branch diff-target is up to date.
Changes applied before test
commit 7593a0368a3fc7c66f4d872ff713cfb0942c116b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jul 27 22:17:59 2020 +0200

    storage*: origin_visit_get(...) -> PagedResult[OriginVisit]
    
    Related to T645

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/589/ for more details.

ardumont edited the test plan for this revision. (Show Details)Wed, Jul 29, 11:31 AM

Build is green

Patch application report for D3627 (id=12803)

Rebasing onto 77960ca4b5...

Current branch diff-target is up to date.
Changes applied before test
commit 39ed4e85d0ffbf8fc78ce729a12b85ca5a108a47
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jul 27 22:17:59 2020 +0200

    storage*: origin_visit_get(...) -> PagedResult[OriginVisit]
    
    Related to T645

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/590/ for more details.

ardumont added inline comments.Wed, Jul 29, 12:22 PM
mypy.ini
63 ↗(On Diff #12803)

Not needed anymore.
I'll drop it.

ardumont added inline comments.Wed, Jul 29, 12:25 PM
swh/storage/in_memory.py
894

??
visits = visits[:extra_limit] should be enough
??

vlorentz requested changes to this revision.Wed, Jul 29, 12:25 PM
vlorentz added inline comments.
swh/storage/tests/test_storage.py
1316

nope (should be opaque)

1330

same

1356

get the next page of results and test it

1358–1360

same

1364

assert actual_page.next_page_token is None

1377

you should get the next page of results and test it too

1612–1615

you forgot this comment

This revision now requires changes to proceed.Wed, Jul 29, 12:25 PM
vlorentz added inline comments.Wed, Jul 29, 12:27 PM
swh/storage/in_memory.py
894

yes. I think I commented something about it yesterday

ardumont added inline comments.Wed, Jul 29, 12:29 PM
swh/storage/in_memory.py
894

about is None, i thought I dropped it entirely but messed it up.

swh/storage/tests/test_storage.py
1316

well, yes but i also said that i wanted to keep the existing tests comparable with the existing ones...
That test is intended to cover the 8 cases of pagination.

If i'm starting to use the opaque identifier here, it will become unreadable...

vlorentz accepted this revision.Wed, Jul 29, 12:30 PM
vlorentz added inline comments.
swh/storage/tests/test_storage.py
1316

meh.

This revision is now accepted and ready to land.Wed, Jul 29, 12:30 PM
ardumont updated this revision to Diff 12807.Wed, Jul 29, 1:29 PM
ardumont marked an inline comment as done.
  • Fix pagination which was off in some cases
  • Adapt according some more remarks in review

Build has FAILED

Patch application report for D3627 (id=12807)

Rebasing onto 77960ca4b5...

Current branch diff-target is up to date.
Changes applied before test
commit c55ff7444b5f99b4e7163742d3e510c356f92d33
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jul 27 22:17:59 2020 +0200

    storage*: origin_visit_get(...) -> PagedResult[OriginVisit]
    
    Related to T645

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/592/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/592/console

ardumont updated this revision to Diff 12809.Wed, Jul 29, 1:33 PM

Fix messed up rebase

Build is green

Patch application report for D3627 (id=12809)

Rebasing onto 77960ca4b5...

Current branch diff-target is up to date.
Changes applied before test
commit fd55b818bf44f417595eb4f81aa659a6756a7510
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jul 27 22:17:59 2020 +0200

    storage*: origin_visit_get(...) -> PagedResult[OriginVisit]
    
    Related to T645

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/594/ for more details.

ardumont added inline comments.Wed, Jul 29, 2:07 PM
swh/storage/tests/test_storage.py
1364

I thought it enough already.

Build is green

Patch application report for D3627 (id=12812)

Rebasing onto f543bd53ee...

Current branch diff-target is up to date.
Changes applied before test
commit 643ebc6e7eb9148558d1c692332785f529ffcb0e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jul 27 22:17:59 2020 +0200

    storage*: origin_visit_get(...) -> PagedResult[OriginVisit]
    
    Related to T645

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/596/ for more details.

ardumont edited the summary of this revision. (Show Details)Wed, Jul 29, 8:25 PM
ardumont edited the summary of this revision. (Show Details)