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

Enable the DAB feature to show create-only serializer #775

Closed

Conversation

AlanCoding
Copy link
Member

Link upstream ansible/django-ansible-base#233

Because we're on DAB devel, I can't update the pyproject.toml just yet. After upstream merges, we can update that and have it take effect.

@AlanCoding AlanCoding requested a review from a team as a code owner March 21, 2024 18:34
@AlanCoding AlanCoding marked this pull request as draft March 21, 2024 18:34
@AlanCoding AlanCoding marked this pull request as ready for review March 26, 2024 13:48
@dhaustein
Copy link
Contributor

This fixes AAP-21919 thank you!

However I noticed a new issue. The response after sending POST api/eda/v1/role_definitions/ to create a new role definition does not contain, for example, the id anymore:

HTTP 201
{
  "permissions": [
    "eda.view_rulebook"
  ],
  "content_type": "eda.rulebook",
  "name": "some random name",
  "description": "some description"
}

@AlanCoding
Copy link
Member Author

Isn't that what you needed? Example of another "..Create" serializer:

            "AwxTokenCreate": {
                "type": "object",
                "properties": {
                    "name": {
                        "type": "string"
                    },
                    "description": {
                        "type": "string"
                    },
                    "token": {
                        "type": "string"
                    }
                },
                "required": [
                    "name",
                    "token"
                ]
            },

And if I patch the DAB code to include the "id" field, it will show in "required", which was the thing this was trying to avoid.

This does break the ability to "follow" the item after creation. AWX gives the full serialization of the item, and a "Location" header. I don't know what solution eda-server has for this problem, or if they have a solution (for a AwxToken for example). The full serialization is incompatible with a create serializer that excludes read-only fields.

@dhaustein
Copy link
Contributor

Not like this. I think it needs more? Here's an excerpt from how /projects/ should behave:

  "get"
    "responses": {
      "schema": {
         "$ref": "#/components/schemas/PaginatedProjectList"
        }
     }

  "post"
    "requestBody": {
      "schema": {
        "$ref": "#/components/schemas/ProjectCreateRequest"
      }
    }
    "responses": {
      "201": {
        "schema": {
          "$ref": "#/components/schemas/Project"
        }
      }
    }

So I guess we need three schemas?

I'm sorry I really can't comment on the actual implementation of EDA API, you'll have to consult @Dostonbek1 or anyone else from @ansible/eda-maintainers

@dhaustein
Copy link
Contributor

@AlanCoding I think we can merge this as a stepping stone and take the discussion about the serializers upstream? What do you think?

@Dostonbek1
Copy link
Member

The approach we have for EDA is that we manually define two different serializers for both request and response on all POST endpoints, this helps us be more explicit on what the expected fields are. The following is an example for Project create endpoint:

    @extend_schema(
        description="Import a project.",
        request=serializers.ProjectCreateRequestSerializer,
        responses={
            status.HTTP_201_CREATED: OpenApiResponse(
                serializers.ProjectSerializer,
                description="Return a created project.",
            ),
        },
    )
    def create(self, request):
        ...

@AlanCoding
Copy link
Member Author

That's not possible to put in DAB (RBAC generally is a stand-alone app and doesn't have drf-spectacular, and neither does AWX). I can try to look into apply the decorator to the role definition viewset inside of eda-server. I worry this borderlines on monkey patching, not sure yet.

@AlanCoding
Copy link
Member Author

I looked at the implementation in

https://github.com/ansible/eda-server/blob/1f5139082a4f4096410d06b5808ac3397f4fc3c2/src/aap_eda/api/views/mixins.py

And this is a hard sell for DAB. It needs both create and partial_update, and it doesn't elegantly defer to the super class. It has to copy and paste the entire method. That's going to be a "no" for a viewset used by AWX and Gateway.

The best solution I could come up in DAB with would be to set a flag on self of the viewset at the start of create or partial_update and use that in get_serializer_class in order to not return the create serializer when the real create or update happens. Even then... still a hard sell.

Otherwise you could re-implement the URL and view and add a bunch of stuff onto it. This would be completely eda-server specific work. I'm somewhat near my limit to contribute here.

@AlanCoding
Copy link
Member Author

I looked at this some more last night, some final notes so that someone else may be able to do the work. I considered both:

  • Using the decorators from drf-spectacular, applying them to the imported role definition view
  • Creating a new custom ANSIBLE_BASE_CUSTOM_VIEW_PARENT class, this would inherit from CreateModelMixin, PartialUpdateOnlyModelMixin, APIView

My conclusion was that both of these steps are necessary to complete the ask here, and that they are probably viable. The decorator seems to set properties on the view method.

https://github.com/tfranzel/drf-spectacular/blob/06d3b47775c766b6b53a578de11c260a77bed844/drf_spectacular/utils.py#L561

Assuming this is all it does (an assumption), no real monkey patching should be necessary. Just import the concrete viewset (and serializers) and apply the decoration.

The custom view parent is necessary so that not just the schema is correct but the behavior is too, like with the response serializer. It might make sense to split the related permission logic out of CreateModelMixin into a separate mixin... maybe you could use the one test_app is using, I don't know.

@AlanCoding AlanCoding marked this pull request as draft March 28, 2024 12:55
@AlanCoding
Copy link
Member Author

Move to a new PR, since the methods changed so dramatically.

AlanCoding added a commit to ansible/django-ansible-base that referenced this pull request Apr 12, 2024
…tency" (#249)

Reverts #233

Because of requirements beyond the anticipated, this solution has become
non-viable to do in DAB to solve the full problem.

The plan is articulated in
ansible/eda-server#775, which is that
`ANSIBLE_BASE_CUSTOM_VIEW_PARENT` will be used to make an
eda-server-standard base, which will copy this code, and combine with
other existing patterns in the code base.

EDIT:

that plan is not implemented in
ansible/eda-server#801
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants