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

[24.0] Fix histories API index_query serialization #17726

Conversation

davelopez
Copy link
Contributor

@davelopez davelopez commented Mar 14, 2024

Follow up to #17717
Fixes #17723

The alternative "index_query" was missing the "serialization_params".
It also adds the "summary" view to the rest of the History Grid listings and it makes this the default serialization view for histories retrieved in "query" mode for consistency with the original "index" endpoint.

Hopefully this time it should make a little difference.

How to test the changes?

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@davelopez davelopez added this to the 24.0 milestone Mar 14, 2024
@davelopez davelopez changed the title [24.0] Fix histories api index serialization [24.0] Fix histories API index_query serialization Mar 14, 2024
@mvdbeek
Copy link
Member

mvdbeek commented Mar 14, 2024

Nice find! Could you add an API test that makes sure we're not getting the detailed view when requesting the summary ?

@davelopez davelopez marked this pull request as draft March 14, 2024 16:02
@davelopez davelopez force-pushed the 24.0_fix_histories_api_index_serialization branch from a93a2a9 to dc6c47e Compare March 14, 2024 16:45
@davelopez davelopez force-pushed the 24.0_fix_histories_api_index_serialization branch from 2bd2dc1 to 1cd3241 Compare March 14, 2024 18:06
@davelopez davelopez marked this pull request as ready for review March 14, 2024 18:07
Copy link
Member

@martenson martenson left a comment

Choose a reason for hiding this comment

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

looks good, thanks!

@martenson
Copy link
Member

martenson commented Mar 14, 2024

the test fails are likely related though

FAILED lib/galaxy_test/selenium/test_dataset_edit.py::TestHistoryPanel::test_history_dataset_update_annotation_and_info - AssertionError
FAILED lib/galaxy_test/selenium/test_uploads.py::TestUploads::test_upload_file - AssertionError: Incorrect number of items in history - expected 1, found 0
assert 0 == 1
FAILED lib/galaxy_test/selenium/test_uploads.py::TestUploads::test_upload_pasted_url_content - AssertionError: Incorrect number of items in history - expected 1, found 0
assert 0 == 1
FAILED lib/galaxy_test/selenium/test_uploads.py::TestUploads::test_upload_composite_dataset_pasted_data - AssertionError: Incorrect number of items in history - expected 1, found 0
assert 0 == 1
FAILED lib/galaxy_test/selenium/test_uploads.py::TestUploads::test_upload_simplest - AssertionError: Incorrect number of items in history - expected 1, found 0
assert 0 == 1
FAILED lib/galaxy_test/selenium/test_uploads.py::TestUploads::test_upload_pair_specify_extension - KeyError: 0

@davelopez davelopez marked this pull request as draft March 15, 2024 08:41
@davelopez
Copy link
Contributor Author

Thanks! Investigating... 🔎

@davelopez
Copy link
Contributor Author

There seems to be some black magic at play here...

WeirdMultiHistoryEffect

This is the effect of 1cd3241 but all responses seem completely fine at first sight and no trace of error anywhere. I'll investigate what the MultiHistory View is doing or expecting regarding this...

@davelopez
Copy link
Contributor Author

Definitely nothing wrong with 1cd3241.
I traced the issue to some undesired computed side effects in the Multi-history view and the historyStore.

I will drop 1cd3241 from here since it is not really related to the serialization problem and open a follow-up PR including that change and addressing the issue in the client.

@davelopez davelopez force-pushed the 24.0_fix_histories_api_index_serialization branch from 1cd3241 to 216036c Compare March 15, 2024 11:34
Until the HistoryMinimal model gets replaced by the CustomHistory again.
@mvdbeek
Copy link
Member

mvdbeek commented Mar 15, 2024

Until the HistoryMinimal model gets replaced by the CustomHistory again.

we should follow up with taking HistoryDetailed but making everything optional again, like we discussed in the backend channel some time ago.

@davelopez
Copy link
Contributor Author

Right! Thanks for remembering, I had already forgot 😅

@davelopez davelopez marked this pull request as ready for review March 15, 2024 12:28
@davelopez
Copy link
Contributor Author

lib/galaxy_test/selenium/test_history_sharing.py::TestHistorySharing::test_shared_with_me is failing with:

File "/home/dlopez/dev/gx-version/24.0/.venv/lib/python3.11/site-packages/anyio/_backends/_asyncio.py", line 851, in run
    result = context.run(func, *args)
             ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/dlopez/dev/gx-version/24.0/.venv/lib/python3.11/site-packages/fastapi/_compat.py", line 125, in validate
    self._type_adapter.validate_python(value, from_attributes=True),
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/dlopez/dev/gx-version/24.0/.venv/lib/python3.11/site-packages/pydantic/type_adapter.py", line 239, in validate_python
    return self.validator.validate_python(__object, strict=strict, from_attributes=from_attributes, context=context)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/dlopez/dev/gx-version/24.0/lib/galaxy/schema/schema.py", line 317, in set_default
    data["model_class"] = literal_to_value(cls.model_fields["model_class"].annotation)
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/dlopez/dev/gx-version/24.0/lib/galaxy/schema/fields.py", line 111, in literal_to_value
    raise Exception("Can't extract default argument for unions")
Exception: Can't extract default argument for unions

I'll see what I can do to fix it.

@davelopez davelopez marked this pull request as draft March 15, 2024 14:10
@mvdbeek
Copy link
Member

mvdbeek commented Mar 15, 2024

Interesting, when I look at the screenshot I'm seeing:
last
so I would have thought it's something about the model definition ?

@davelopez
Copy link
Contributor Author

davelopez commented Mar 15, 2024

It seems to be related to when requesting only specific keys the model_class is optional so instead of Literal["History"] we get Optional[Literal["History"]]... I still don't know how any of these changes affect here... 🤔

image

image

Update

I thought the request was failing because of this, but the failure was me trying to work on 2 things at the same time and mixing branches 🤦

The test failed because it was expecting the username to be present in the listing and that was not part of the summary view as hinted in Marius' screenshot.

I need a break 😅

@davelopez davelopez marked this pull request as ready for review March 15, 2024 16:27
@davelopez
Copy link
Contributor Author

davelopez commented Mar 15, 2024

Tests are passing now. The failing Toolshed one is unrelated.

@mvdbeek mvdbeek merged commit e2e6ba4 into galaxyproject:release_24.0 Mar 17, 2024
52 of 53 checks passed
@davelopez davelopez deleted the 24.0_fix_histories_api_index_serialization branch March 18, 2024 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants