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

Respect parent lookup regex value (sending parent viewset to NestedRegistryItem) #87

Closed
wants to merge 2 commits into from

Conversation

frwickst
Copy link
Contributor

A try for this was made in #63 but it did not lead anywhere and this is really starting to become a problem for me so hopefully this is good enough :)

Problem

Say that you have a user route (/users) and you do not want to expose the id value of the user, so you use the users email instead. Making a detailed route look like /users/frwickst@gmail.com. You test the route and notice that you get a 404 page. This is because the default lookup regex is [^/.]+, and this works fine for most routes, but not when there is a dot (.) in the route. And since dots are valid in a url we this is a shame. The solution to this is to set your lookup_filed to 'email' and write your own lookup_value_regex in your view. In this case we could set it to something fancy such as [^@]+@[^@]+\.[^@/]+. And voilà, the route now works.

The problem is that this regex value is not respected in the nested routers that are included in drf-extensions, instead the hard coded value of [^/.]+ is used.

Solution

By default we do not have access to the parents regex value as it is stored in the parents viewset, which we don't have access to either. This pull requests lets the viewset of the parent be passed to the ´NestedRegistryItem´ class, exposing all viewset functions and variables, including the regex. The regex is then used when creating the get_parent_prefix(), and falls back on the default value of [^/.]+ if no regex is set.

Caveat

There is a second thing here to keep in mind. If using custom lookup_field values in the view, one also needs to set the lookup_url_kwarg value when using nested router, in the example above it would be lookup_url_kwarg = 'parent_lookup_email'. This has to do with the way the NestedViewSetMixin is built, and can probably be fixes as well (just not in this pull request).

@chibisov
Copy link
Owner

chibisov commented Jun 2, 2015

I'll look at this pull request as soon as possible

@clintonb
Copy link

I, too, need this functionality added to make drf-extensions usable for my API.

@clintonb
Copy link

@chibisov Any update on merging this PR?

@chibisov
Copy link
Owner

I'll merge it in next two days. Sorry for the delay.

@frwickst
Copy link
Contributor Author

frwickst commented Aug 4, 2015

Ping!

If you are no longer working on drf-extensions it might be a good idea to create a community around it and give more people write access to the repository.

@clintonb
Copy link

@chibisov is this project still alive?

@frwickst
Copy link
Contributor Author

Wow... so you are taking my code, without taking my commits. Basically leaving out of the fix. This is not OK with me, please include my commit and add your own code as separate commits.

@chibisov
Copy link
Owner

This is first time somebody asks me to do that :) I'm just curious, for what?

@frwickst
Copy link
Contributor Author

Really, so you have done this before?

This is how open source software works, else there will be no record of me contributing to the code base. It is like me giving you something, and you telling everyone you made it, the way you have done it now. GitHub provides a merge button for a reason and the page even says "Closed with unmerged commits" why do you think that is?

chibisov pushed a commit that referenced this pull request Aug 29, 2015
@chibisov
Copy link
Owner

I've merged your commits. Is that ok now?

@frwickst
Copy link
Contributor Author

Yes, thank you. But please don't do anything like this in the future, ever. The code that people give to you are written by people wanting to help. Not including them in the commit history leaves no track of their contribution and feels like a slap in the face to many.

Think of it like me taking your repo, and then I change all the commits to be mine instead of yours, I don't think you would like that very much. It would be basically me stealing your code. That is, in a smaller scale, what you would be doing by including other peoples code but not giving them credit in any way. If you absolutely must include other peoples code without including their commits, then at least include them in the commit message or in the code as comments in some way.

@chibisov
Copy link
Owner

Never again! ✊

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.

None yet

3 participants