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 500 on unintended endpoints and further refactor association router #370

Merged
merged 28 commits into from
May 21, 2024

Conversation

AlanCoding
Copy link
Member

@AlanCoding AlanCoding commented May 8, 2024

Something drove me crazy after testing with my last patch - which is that certain serializers (with the same name) were created twice. This is logical if you think about what's being done. This PR adds a registry to fix the problem.

It also trades a few things in code organization. It gets rid of the "factory" method, and adds a new utility method for getting the parent class, which has to follow a certain format. Then the registry management becomes the main job of get_serializer_class

Recapping of major changes being done here:

  • Serializers for association & disassociation are refactored so that they don't do "rbac" filtering by their own logic. This change gets rid of the user-specialized association serializer using visible_users and the general access_qs call in favor of calls to filter_queryset.
  • As a subpoint of the above point, get_serializer_class no longer builds querysets (for instances). This changes it to use methods from the view instance inside of the serializer context. This has to happen on the serializer __init__.
  • Disassociation validation that the object is already attached is deleted, replaced by queryset logic, which now uses the same get_queryset as the listing does, which only includes attached objects to begin with.
  • New parent class for related model viewsets is added which is inserted and lower precedence than the passed-in class. This means that methods defined here can be overwritten in order to customize behavior. These are listed in docs too.
  • When synthesizing the new association viewset class, all methods other than list are disabled by a metaclass. This avoids the duplicate registration I complained above above.
  • A serializer registry is added so that we don't create the same-name class multiple times, which confuses OpenAPI.

@john-westcott-iv john-westcott-iv changed the title Fix 500 on unintended endpoints and further refactor association router [WIP] Fix 500 on unintended endpoints and further refactor association router May 9, 2024
@AlanCoding AlanCoding requested a review from relrod May 9, 2024 16:55
@AlanCoding AlanCoding changed the title [WIP] Fix 500 on unintended endpoints and further refactor association router Fix 500 on unintended endpoints and further refactor association router May 9, 2024
@AlanCoding AlanCoding marked this pull request as ready for review May 9, 2024 18:03
@AlanCoding AlanCoding added the Ready for review This PR is ready for review either initially or comments have been address label May 16, 2024
@AlanCoding
Copy link
Member Author

I've created #393 to discuss 2 remaining things I'm still not happy with. But I don't want those in scope here.

@john-westcott-iv
Copy link
Member

@AlanCoding to make a merge meeting for this PR sometime next week.

@AlanCoding AlanCoding force-pushed the only_list2 branch 2 times, most recently from ecdfb87 to 391e703 Compare May 20, 2024 14:22
docs/lib/routers.md Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented May 21, 2024

@AlanCoding AlanCoding merged commit 251ede8 into ansible:devel May 21, 2024
8 checks passed
AlanCoding added a commit that referenced this pull request May 23, 2024
I only just merged
#370 but this fixes a
problem with it.

This was encountered using drf-spectacular decorators to resolve very
boutique eda-server issues, which could result in the traceback:

```
  File "/home/alancoding/venvs/eda/lib64/python3.11/site-packages/drf_spectacular/drainage.py", line 168, in get_view_method_names
    return [
           ^
  File "/home/alancoding/venvs/eda/lib64/python3.11/site-packages/drf_spectacular/drainage.py", line 169, in <listcomp>
    item for item in dir(view) if callable(getattr(view, item)) and (
                                           ^^^^^^^^^^^^^^^^^^^
  File "/home/alancoding/repos/awx/testing/django-ansible-base/ansible_base/lib/routers/association_resource_router.py", line 231, in attribute_raiser
    raise AttributeError
AttributeError
```

This traceback is a problem even when you have no other context.
You can see `item for item in dir(view)`. Then one of those items gives
an `AttributeError`. I mean, the `dir` method is pretty much, by
definition, the set of attributes that won't give you an attribute
error.

Non-obvious, but since all of this is done inspecting the view _class_,
similar to the AttributeError, all the fixes have to be done on the
metacalss.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for review This PR is ready for review either initially or comments have been address
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants