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

Show related associations in OPTIONS for attach-detach #14428

Draft
wants to merge 9 commits into
base: devel
Choose a base branch
from

Conversation

AlanCoding
Copy link
Member

SUMMARY

This is a supporting change that would be used in the following features:

Ping @sean-m-sullivan

I mentioned this in the CLI change I had started on, but said there that I wasn't doing it the "right" way, since the metadata had to loop over all views... and this was pretty objectively bad. This PR is intended to off that "right" way.

The only drawback to doing it the right way, is that we have to restructure our serializers in order to give ourselves something other than a kludge of "whatever" type of links. So the idea is that this structures the related links.

Trying to limit the scope, this grouping only includes related lists which are the attach-detach kind, and which are editable.

Note that up-until-this, we also had related lists that were editable and ForeignKeys. These were bugs, and are fixed here. Disassociation in that case almost always created bad data (orphaned objects), and some times possibly worse. This is what one of the 2 new tests is enforcing.

ISSUE TYPE
  • Bug, Docs Fix or other nominal change
COMPONENT NAME
  • API

@AlanCoding
Copy link
Member Author

Example, seen in OPTIONS for /api/v2/organizations/1/

    "related_associations": [
        [
            "users",
            "users"
        ],
        [
            "admins",
            "users"
        ],
        [
            "notification_templates_started",
            "notification_templates"
        ],
        [
            "notification_templates_success",
            "notification_templates"
        ],
        [
            "notification_templates_error",
            "notification_templates"
        ],
        [
            "notification_templates_approvals",
            "notification_templates"
        ],
        [
            "instance_groups",
            "instance_groups"
        ],
        [
            "galaxy_credentials",
            "credentials"
        ]
    ],

Note that with this approach, I could totally apply it to /api/v2/organizations/ as well. I think there's a good argument for that too... so I'm probably in favor of it.

@sean-m-sullivan
Copy link
Contributor

I like it,
I tried to get this deploy using your branch, so I could see the api, and it is giving errors.
Having the associations for an endpoint would definitely help.

I can rework that PR to use this, or other additions you want, may try another improvement I thought of to my code once this is worked out, but I know I've been asked by a few people the last few weeks about the state of that PR.

@AlanCoding AlanCoding changed the title Related associations Show related associations in OPTIONS for attach-detach Sep 11, 2023
@AlanCoding
Copy link
Member Author

@sean-m-sullivan you made the point in the meeting earlier that a mapping might be preferable to a list here.

One argument for a mapping is that we could "label" different properties. So let me back up and describe what data I'm trying to put in here to begin with. Consider an organization, that has:

"url": "/api/v2/organizations/1/",
"related": {
    "named_url": "/api/v2/organizations/Default/",
    "created_by": "/api/v2/users/1/",
    "modified_by": "/api/v2/users/1/",
    "execution_environments": "/api/v2/organizations/1/execution_environments/",
    "projects": "/api/v2/organizations/1/projects/",
    "inventories": "/api/v2/organizations/1/inventories/",
    "job_templates": "/api/v2/organizations/1/job_templates/",
    "workflow_job_templates": "/api/v2/organizations/1/workflow_job_templates/",
    "users": "/api/v2/organizations/1/users/",
    "admins": "/api/v2/organizations/1/admins/",
    "teams": "/api/v2/organizations/1/teams/",
    "credentials": "/api/v2/organizations/1/credentials/",
    "applications": "/api/v2/organizations/1/applications/",
    "activity_stream": "/api/v2/organizations/1/activity_stream/",
    "notification_templates": "/api/v2/organizations/1/notification_templates/",
    "notification_templates_started": "/api/v2/organizations/1/notification_templates_started/",
    "notification_templates_success": "/api/v2/organizations/1/notification_templates_success/",
    "notification_templates_error": "/api/v2/organizations/1/notification_templates_error/",
    "notification_templates_approvals": "/api/v2/organizations/1/notification_templates_approvals/",
    "object_roles": "/api/v2/organizations/1/object_roles/",
    "access_list": "/api/v2/organizations/1/access_list/",
    "instance_groups": "/api/v2/organizations/1/instance_groups/",
    "galaxy_credentials": "/api/v2/organizations/1/galaxy_credentials/"
},

Here, we are talking about adding data to help the client in OPTIONS requests to either /api/v2/organizations/ or /api/v2/organizations/1/. When the user looks at this "related" dictionary, it's completely unclear what function each link provides.

The current PR tries to provide a list of what entries in "related" can attach and detatch from the parent object, which is organization id=1 here.

Each entry in "related_associations" key in OPTIONS is a tuple, and the first element is the key in the related dictionary and the second element is the global endpoint for that type. To put that in concrete terms, we have a "related" entry of:

"related": {
    "admins": "/api/v2/organizations/1/admins/",

That keys to this:

        [
            "admins",
            "users"
        ],

So this is saying "For the related entry under the admins key, you can find the resource type at /api/v2/users/".

It's true this isn't the only piece of data someone might want, because it doesn't clarify that the related endpoint is /api/v2/organizations/1/admins/, which is actually very non-trivial. The tail of this URL tends to be the same string as the key in "related"... but this was never guaranteed.

Up to now, we have formalized 3 pieces of data that a client might want/need:

  • the key in the "related" links
  • the tail of the association-diassocation endpoint, like /api/v2/organizations/1/TAIL/
  • the global URL for the relevant resource type like /api/v2/RESOURCE/

But how can all this detail be communicated? A mapping instead of a list? But then it become ambiguous what the key is. I don't have a clean answer, so maybe let's move on to propose a list data structure, like

[
  {
    "key": "admins",
    "tail": "admins",
    "api": "users"
  }

While I hate the "api" name, I don't want to use the word "type" because that already means something else (likely different) in another context.

I'm still very far from finalizing this, maybe this aesthetic might be nice?

[
  {
    "key": "admins",
    "tail": "/api/v2/organizations/pk/admins/",
    "api": "/api/v2/users/"
  }

The problem there is obviously the "pk". Since we're discussing add this to OPTIONS for /api/v2/organizations/, we can't give a concrete pk. Nonetheless, I strongly see the argument for giving full URLs for clarity. In fact, I would agree with dropping the "key" here, because the "related" dictionary is just informational, and clients can go construct the endpoints for themselves.

Add disassociate command and clean up interface

Limit new OPTIONS data to the detail views
Refactor serializers such that editable m2m links
  are defined in static lists, which is reused by metadata

Add two corresponding tests that enforce that related
  attach-detatch views are listed in those static lists

As a consequence, we have to correct some view parent classes
  as they would allow attach-detatch where it was not valid
@AlanCoding
Copy link
Member Author

What I have now looks like this:

    "associations": [
        {
            "relative_url": "users",
            "related_entry": "users",
            "resource": "users"
        },
        {
            "relative_url": "admins",
            "related_entry": "admins",
            "resource": "users"
        },
        {
            "relative_url": "notification_templates_started",
            "related_entry": "notification_templates_started",
            "resource": "notification_templates"
        },

I tried showing URLs in these entries, but experienced extreme hesitation, because simply no other item in OPTIONS are URLs. They would get auto-linked in the API browser which would be... questionable. In general though, I feel like OPTIONS should deal with abstract information for the view without specifics. We already do link these, and it's in the related. To link it in OPTIONS would be duplicating functionality. By that argument, I (1) am not using related URLs and (2) convinced myself to include "related_entry", as we mean to be deferential to the fact that the related links provides a concrete link. The client should already have that data, or could easily get it.

@AlanCoding AlanCoding marked this pull request as ready for review September 19, 2023 14:41
@jbradberry jbradberry self-requested a review December 4, 2023 15:04
@dmzoneill dmzoneill marked this pull request as draft February 14, 2024 16:50
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.

None yet

2 participants