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

Fixed issues with schema name collisions #5486

Merged

Conversation

mlubimow
Copy link
Contributor

@mlubimow mlubimow commented Oct 7, 2017

If urls include elements in ['retrieve', 'list', 'create', 'update', 'partial_update', 'destroy'] schema generation can fail (if a nested item clashes with a previously definedLink) or replace other link.

PR fixes #5484 and #4704.

Also took unit tests from another PR targeting this issue: #5464

@carltongibson
Copy link
Collaborator

carltongibson commented Oct 8, 2017

OK. This looks interesting. I need to review properly.

Can you describe what’s going on with LinkNode and the helper methods?

How does this affect the output schema? How does it affect the rendered docs?

👍🏽

@mlubimow
Copy link
Contributor Author

mlubimow commented Oct 8, 2017

@carltongibson I optimized my solution a bit.

So the issue was that nodes in paths tree could be either OrderedDict or Link - which was causing issue in two cases:

  1. Node is both link and subpath of another link
  2. Multiple links were assinged to node

So what I've done, I've made every node in a tree a LinkNode (ordered dict with links array). So each node now can be both a link and a subpath. Also, because I've got array of links, now we can have multiple links in same node (same path said in other way).

How it affects the schema? It doesnt use retrieve , read etc as keys now, its just get_0, get_1, post_0 and so on. How it affect docs? I've checked swagger only because this is what Im working with and it works fine, for other UIs - I dont know.

@@ -446,17 +446,17 @@ def test_schema_for_regular_views(self):
coreapi.Field('ordering', required=False, location='query', schema=coreschema.String(title='Ordering', description='Which field to use when ordering the results.'))
]
),
'custom_list_action': coreapi.Link(
'get_1': coreapi.Link(
Copy link
Collaborator

@carltongibson carltongibson Oct 9, 2017

Choose a reason for hiding this comment

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

Can we avoid all these renames?

Copy link
Contributor Author

@mlubimow mlubimow Oct 9, 2017

Choose a reason for hiding this comment

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

Surely not every time because that is a part of the issue, they clash sometimes. Are they used anywhere? I thought they are just ids

Copy link
Collaborator

@carltongibson carltongibson Oct 9, 2017

Choose a reason for hiding this comment

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

They're used in the interactive API documentation.

Can we keep them as are if there is no collision and only append a number when there is more than one?

i.e. here, if there were a collision, we'd have custom_list_action and custom_list_action_1 and so on.

Copy link
Contributor Author

@mlubimow mlubimow Oct 9, 2017

Choose a reason for hiding this comment

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

We can do it

@mlubimow mlubimow force-pushed the master---schema-naming-collision branch from 30553f7 to 50b3c05 Compare Oct 9, 2017
@mlubimow
Copy link
Contributor Author

mlubimow commented Oct 9, 2017

@carltongibson should be fine now

Copy link
Collaborator

@carltongibson carltongibson left a comment

OK. This is looking really good. I need a day or so to review fully (looking at the Interactive Docs by hand to make sure all is OK).

Given existing behaviour should be unchanged, we can go for a quick point release.

Exciting stuff. Thanks for the effort!


def test_is_list_view_recognises_retrieve_view_subclasses():
Copy link
Collaborator

@carltongibson carltongibson Oct 11, 2017

Choose a reason for hiding this comment

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

Why have you removed this test case? I assume it's a mistake...

It's for a separate issue. #5165

Copy link
Collaborator

@carltongibson carltongibson left a comment

OK. This is good.

Pulling it into a project using DRF's interactive docs, it behaves well.

Can you just replace the test case from #5165 at the bottom?

I think then we're good to go.

Great one. Thanks.


def test_is_list_view_recognises_retrieve_view_subclasses():
Copy link
Collaborator

@carltongibson carltongibson Oct 11, 2017

Choose a reason for hiding this comment

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

We need this test case.

Copy link
Contributor Author

@mlubimow mlubimow Oct 12, 2017

Choose a reason for hiding this comment

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

strange its got removed, I will add it ofc

@carltongibson carltongibson added this to the 3.7.1 Release milestone Oct 11, 2017
@mlubimow
Copy link
Contributor Author

mlubimow commented Oct 12, 2017

@carltongibson Done!

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.

Views not included (replaced) in schema under certain conditions
2 participants