Fixes: #476 - Overriden primary key in links

[kopf]

Hey, here's my solution for #476. I hope it's acceptable. If I've forgotten anything please let me know!

All enabled tests are passing on the branch:

awalton@tgv:~/work/flask-restless:overriden_primary_key_in_links  V $ py.test           (restless)
======================================= test session starts ========================================
platform darwin -- Python 2.7.10, pytest-2.9.0, py-1.4.31, pluggy-0.3.1
rootdir: /Users/awalton/work/flask-restless, inifile:
collected 449 items

tests/test_bulk.py sssssss
tests/test_creating.py ..........................................ss..
tests/test_deleting.py s............
tests/test_fetching.py ............sss................................................s...
tests/test_filtering.py s....................................................
tests/test_functions.py .........
tests/test_manager.py ......................s..
tests/test_metadata.py s.
tests/test_serialization.py ..................
tests/test_updating.py .................................sssss..
tests/test_updatingrelationship.py ..................................................
tests/test_validation.py ............
tests/test_jsonapi/test_creating_resources.py ........
tests/test_jsonapi/test_deleting_resources.py ..
tests/test_jsonapi/test_document_structure.py .....................
tests/test_jsonapi/test_fetching_data.py ...............................................
tests/test_jsonapi/test_server_responsibilities.py s......
tests/test_jsonapi/test_updating_relationships.py ..........
tests/test_jsonapi/test_updating_resources.py ............

============================= 426 passed, 23 skipped in 11.03 seconds ==============================

[jfinkels]

I prefer to see raise ValueError here, without the parentheses.

[jfinkels]

I think maybe the helper function primary_key_name should be removed and its functionality placed in the primary_key_for function, so that the code for finding a primary key name is not split over multiple functions. Can it be rearranged that way?

Also, there are a couple other places where primary_key_for should be used, essentially wherever you see primary_key_name (in views/base.py and serialization.py).

[kopf]

Regarding removing primary_key_name, this is made difficult by the fact that the current PrimaryKeyFinder.__call__ function calls itself recursively, and may return None (when the APIInfo object has been created with primary_key=None).

So, in order to do this, the design of the PrimaryKeyFinder class needs to deviate slightly from the other Finder classes. Two possibilities occur to me immediately:

• What's currently in PrimaryKeyFinder.__call__ is moved into PrimaryKeyFinder.find(), and what's currently in the primary_key_name is moved in to the __call__ function.
• Instead of creating a new function PrimaryKeyFinder.find() at the class level, this logic is instead placed in a nested function within the __call__ function.

Do you have any preference, or do you have a better idea?

[kopf]

I've implemented the latter solution (a nested function within PrimaryKeyFinder.__call__) in fbae56f - let me know what you think.

[jfinkels]

Remove this and add the line from .helpers import loads below the line from .helpers import force_content_type_json. (This loads flask.json.loads instead of json.loads.)

[kopf]

Ahaa! This is what caused the py3 breakages!

Sorry, I still haven't had much time to venture into python 3 yet :(

[jfinkels]

How about this

class PrimaryKeyFinder(KnowsAPIManagers, Singleton):
    """The singleton class that backs the :func:`primary_key_for` function."""

    def __call__(self, instance_or_model, _apimanager=None, **kw):
        if isinstance(instance_or_model, type):
            model = instance_or_model
        else:
            model = instance_or_model.__class__

        if _apimanager is not None:
            managers_to_search = [_apimanager]
        else:
            managers_to_search = self.created_managers
        for manager in managers_to_search:
            if model in manager.created_apis_for:
                primary_key = manager.primary_key_for(model, **kw)
                break
        else:
            message = ('Model "{0}" is not known to {1}; maybe you have not'
                       ' called APIManager.create_api() for this model?')
            if _apimanager is not None:
                manager_string = 'APIManager "{0}"'.format(_apimanager)
            else:
                manager_string = 'any APIManager objects'
            message = message.format(model, manager_string)
            raise ValueError(message)

        # If `APIManager.create_api(model)` was called without providing
        # a value for the `primary_key` keyword argument, then we must
        # compute the primary key name from the model directly.
        if primary_key is None:
            pk_names = primary_key_names(model)
            primary_key = 'id' if 'id' in pk_names else pk_names[0]
        return primary_key

[kopf]

Am personally not the biggest fan of for: else: but I don't see many feasible alternatives here, due to the contents of the for loop. Looks good! 👍

[jfinkels]

Haha no one is a fan of for/else! I think it works in this situation.

[kopf]

OK, have pushed your solution to the branch. Sorry about the wait.

[jfinkels]

Sorry for the inconvenience, but I just merged in some changes that makes this un-pullable. Can you git rebase -i master, resolve the conflicts, and squash your commits into a single commit (with a good commit message)?

[kopf]

No worries - but I'm having a crazy week here. I'll try to get around to it tomorrow, if not, it'll be friday by the time I get to do it.

[kopf]

OK, managed to get around to it tonight. Commits are squashed into 69f38ad.

[jfinkels]

Thank you!!!

[kopf]

My pleasure! If you know of other issues that are blocking the 1.0 release that I might be able to help out on, just ping me on them, and I'll try to devote some time to them on fridays.

[jfinkels]

Issue #480. :)