multimeric / flapison

Flask extension to build REST APIs around JSONAPI 1.0 specification.
http://flask-rest-jsonapi.readthedocs.io
MIT License
13 stars 4 forks source link

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

Open srhinos opened 4 years ago

srhinos commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

srhinos commented 4 years ago

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 commented 4 years ago

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