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

Badly Architected Resource CRUD handlers makes it impossible to customize them #14

Open
srhinos opened this issue Dec 18, 2019 · 9 comments

Comments

@srhinos
Copy link

srhinos commented Dec 18, 2019

Planning on submitting a PR to resolve this but I wanted to make this a possible function of the library:
GET /api/label/<string:label_name>
along with the ability to create relationships this way too to cut down on my frontend needing to make an extra get request to match up IDs to functions.

The issue comes in ResourceDetail's patch function:
https://github.com/TMiguelT/flapison/blob/master/flapison/resource.py#L327

The get function allows the kwargs to be modified in the before_get function but its broken in patch if you're trying to modify the kwargs in either before_patch or before_marshmallow due to this function (for some reason) relying solely on data from the request and now enabling it to be modified before doing queries on it.
https://github.com/TMiguelT/flapison/blob/master/flapison/resource.py#L353

Proposal:

Move before_patch to the top of the file as it should run before the patch function does.

Note:

This is the same issue for ResourceList's post function so I'll be submitting a fix for both
https://github.com/TMiguelT/flapison/blob/master/flapison/resource.py#L222

@multimeric
Copy link
Owner

So in summary you want to move the before_x hooks to the start of their respective handler functions? This sounds reasonable. Out of interest, was this broken upstream or is this a regression in flapison?

@srhinos
Copy link
Author

srhinos commented Dec 18, 2019

yeah, broken upstream as well.

Like I said scenario is having the ability to PATCH ModelA with an instance of ModelB in a many field

class Model1Detail(ResourceDetail):
    def before_get(self, view_kwargs):
        if view_kwargs.get("display_name") is not None:
            model1 = Model1.find_by(display_name=view_kwargs.get("display_name"))
            view_kwargs["id"] = model1.id

    # Doesn't work but included as example
    # def before_patch(self, view_args, view_kwargs):
    #     if view_kwargs.get("display_name") is not None:
    #         model1 = Model1.find_by(display_name=view_kwargs.get("display_name"))
    #         view_kwargs["id"] = model1.id

    schema = Model1Schema
    data_layer = {
        "session": ext.db.session,
        "model": Model1,
        "methods": {"before_get": before_get},
    }

with this as a view

    api.route(
        Model1Detail,
        "model1_detail",
        "/api/model1/<int:id>",
        "/api/model1/<string:display_name>",
    )

so now I can

PATCH api/model1/cool_name
{
  "data": {
    "type": "model1",
    "name": "cool_name",
    "relationships": {
      "threads": {
        "data": [
          {
            "type": "model2",
            "id": "1"
          }
        ]
      }
    }
  }
}

I'd have to handle the backend of doing the look-up to enable this to function but at the end of the day, I save a good amount of work by enabling string buckets thru flapison and not having to manually define a thousand endpoints for specific use cases

@srhinos
Copy link
Author

srhinos commented Dec 18, 2019

I think this is breaking unit tests tho, if you have a better way of approaching this, let me know and I can work on it in some spare time

@multimeric
Copy link
Owner

multimeric commented Dec 18, 2019

If you want to access resources using a name rather than an ID, could you not just override get_object? The issue with your current approach is that you're querying the same row twice: once to find the ID from the name, and once to GET/PATCH/DELETE the row using its ID.

Having said this, I still think this is a relevant issue, because there might be use-cases where there is no alternative approach.

@srhinos
Copy link
Author

srhinos commented Dec 18, 2019

Man, those functions aren't really well documented as existing, are they? I honestly keep getting more functionality from src diving than reading the docs

@srhinos
Copy link
Author

srhinos commented Dec 18, 2019

You're right though, but I do also agree, the before_x should be ran before x as expected because some extra work might want to be done before said function is called and not rely on data that can't be modified within said functions

@multimeric
Copy link
Owner

I would welcome a documentation PR. Also I really need to create a documentation website build like they have upstream.

@srhinos
Copy link
Author

srhinos commented Dec 18, 2019

For sure, so looking into this issue more, it doesnt look like any before_x_object or before_x_relationship functions will solve this as its throwing the error before those are called in the stack and is pulling the data its checking directly from the request.json() so even if it was being called before, I wouldn't have the ability to modify it

Was super pumped to resolve the block haha

@multimeric
Copy link
Owner

Can you post an actual trace of the error you're getting? Also, would get_object not work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants