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 naming collisions in Schema Generation #5464

Merged

Conversation

carltongibson
Copy link
Collaborator

@carltongibson carltongibson commented Sep 29, 2017

Closes #4704

WIP DO NOT MERGE

If urls include elements in ['retrieve', 'list', 'create', 'update', 'partial_update', 'destroy'] — I **think** just those — schema generation can fail is a **nested** item clashes with a previously defined Link`.

(We want a dictionary to insert a Link to, but a Link is already there.)

Thus far, just added a couple of failing test case for the reported cases.

@tomchristie
Copy link
Member

tomchristie commented Oct 3, 2017

Okay, so the easiest way to review this would be to comment out in this ticket what the simplest example of a schema collision is. In the shown case, do we have two different links that are both supposed to be included as this?...

{
    "retrieve": Link()
}

If so, what would we expect the schema structure to look like instead?

I suppose we could do something like fall back to...

{
    "retrieve": Link()
    "test_retrieve": Link()
}

Tho it's not obvious how we'd make that work reliably.

Alternatively, could we raise a clear exception explaining what the issue is, and document this as a constraint? (allow the user to either renamed the URL or customize the structure generation?)

@carltongibson
Copy link
Collaborator Author

carltongibson commented Oct 5, 2017

I've adjusted insert_into to raise a ValueError with a helpful message here. It's not perfect but:

... it's not obvious how we'd make that work reliably.

I'd be happy to see a follow-up PR here.

@tomchristie
Copy link
Member

tomchristie commented Oct 5, 2017

I think we should role with this approach for now, yup.

(And yes, we can still follow-up on it later)

@carltongibson carltongibson force-pushed the 37/schema-naming-collisions branch from f24cf68 to a73aa6b Compare Oct 5, 2017
@carltongibson carltongibson merged commit d138f30 into encode:master Oct 5, 2017
1 check passed
@carltongibson carltongibson deleted the 37/schema-naming-collisions branch Oct 5, 2017
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