pimcore / data-hub

Data delivery & consumption platform for Pimcore.
Other
125 stars 107 forks source link

DataObject Listings return parents, too #257

Closed Iblis closed 4 years ago

Iblis commented 4 years ago

Expectation: I only get 'myAwesomeCompany ' returned. Actual Behaviour: I get all 3 companies returned in that list.

Also, I am able to delete the parents via Mutation when I have allowed Deletion on path "/companies/germany/". My expectation would have been that I can only delete objects on, or actually below the path, not above it.

weisswurstkanone commented 4 years ago

Could you please provide a sample query which can be reproduced on demo?

Iblis commented 4 years ago

Since this is about how your hierachy is set up, I am not sure if this can be reproduced in the demo just like that. Or am I missing something? I could try to setup the hierachy with eg cars from the demo and describe that.

Iblis commented 4 years ago

In this example, the car with Id 81 is the 'Cobra 427'. Interestingly, its values are null except for the id. I don't think I had this behaviour on my own instance. Anyhow, I would expect that this entry would not show up at all.

weisswurstkanone commented 4 years ago

thanks, could you please share your var/config/datahub-configurations.php file ?

weisswurstkanone commented 4 years ago

I don't see where the query/listing is restricted to to a parent.

Please have a look at https://github.com/pimcore/data-hub/blob/master/doc/graphl/Filtering.md

Iblis commented 4 years ago

While playing around with it a bit, we found a case in the demo data that doesn't require adding additional data. Here is our configuration (as txt so that Github allows the attachment), the change is in line 2967: "cpath" => "/Product Data/Cars/vw/Käfer/1200", datahub-configurations.txt

Whith the following query: {getCarListing{edges{node{ name classname carClass id }}}} We get the following result:

{
  "data": {
    "getCarListing": {
      "edges": [
        {
          "node": {
            "name": null,
            "classname": null,
            "carClass": null,
            "id": "208"
          }
        },
        {
          "node": {
            "name": "Käfer",
            "classname": "Car",
            "carClass": "Economy car",
            "id": "209"
          }
        },
        {
          "node": {
            "name": "Käfer",
            "classname": "Car",
            "carClass": "Economy car",
            "id": "212"
          }
        },
    ...
      ]
    }
  }
}

Actual: The parent 'Käfer' is returned with null values Expected: The parent 'Käfer' (id:208) is not part of the list.

Even more interesting, when adding object bricks to the query (eg SaleInformation~availabilityPieces, also part of the attached data-hub config), we get errors. Query: {getCarListing{edges{node{ name classname carClass id availabilityPieces }}}} Result:

{
  "errors": [
    {
      "message": "Internal server error",
      "extensions": {
        "category": "internal"
      },
      "locations": [
        {
          "line": 1,
          "column": 55
        }
      ],
      "path": [
        "getCarListing",
        "edges",
        0,
        "node",
        "availabilityPieces"
      ]
    }
  ],
  "data": {
    "getCarListing": {
      "edges": [
        {
          "node": {
            "name": null,
            "classname": null,
            "carClass": null,
            "id": "208",
            "availabilityPieces": null
          }
        },
        {
          "node": {
            "name": "Käfer",
            "classname": "Car",
            "carClass": "Economy car",
            "id": "209",
            "availabilityPieces": null
          }
        },
        {
          "node": {
            "name": "Käfer",
            "classname": "Car",
            "carClass": "Economy car",
            "id": "212",
            "availabilityPieces": null
          }
        },
        {
          "node": {
            "name": "Käfer",
            "classname": "Car",
            "carClass": "Economy car",
            "id": "215",
            "availabilityPieces": 3
          }
        },
      .....
      ]
    }
  }
}

With latest changes from data-hub dev-master, this even turns into a full 500 response.

When we change the path to "cpath" => "/Product Data/Cars/vw/Käfer", the error disappears and the parent 'Käfer' (id: 208) is returned correctly (field values are their actual values).

From our perspective, something is wrong with how parents are treated in those lists in regards to the object hierachy. But maybe we didn't understand the concept well? From your response, it sounds as if parents are supposed to be returned as long as we don't do client-side filtering. But then I don't understand why the parent values are all null, what the pathes are for in the security definitions and why there are errors with object bricks when setting the 'cpath' to an object deeper in the hierachy (like "/Product Data/Cars/vw/Käfer/1200").

Iblis commented 4 years ago

@weisswurstkanone any hints on the issue discovered?

weisswurstkanone commented 4 years ago

Thanks for your investigations! Fixed via https://github.com/pimcore/data-hub/commit/84d99b5cfd80ce993aacab3adc185276594c68f8

Iblis commented 4 years ago

Thanks a lot! What I realized is that I only emphasized on the parent on the top. The parent that is ON the path might be a discussion topic, too.

So now, when I call {getCarListing{edges{node{ name classname carClass id }}}} I won't get the Car with id 208 anymore, but still 209. Which sounds reasonable because it is on the defined path. But when I activate the delete mutation and execute it, I am able to delete the object with id 209. Is this the intended behaviour? So if I specify the path "cpath" => "/Product Data/Cars/vw/Käfer/1200" and give permission to delete for that path, it means I am able to not just delete all objects on that path, but even the '1200' node itself. Personally, this sounds a bit unintuitive to me because I would expect giving permissions for everything below the path. But I can see how one could argue that if we give access to this node, you have full access, even deleting it. So could you clarify if the current behaviour is intended/expected?

weisswurstkanone commented 4 years ago

Can you provide an example for the delete mutation on demo ?

Iblis commented 4 years ago

I tried, in my example above I am refering to the same example as in my comment further above (https://github.com/pimcore/data-hub/issues/257#issuecomment-670564909). The only difference is to add a mutation for Car and allow delete. But I can also give you the corresponding data-hub config later today

weisswurstkanone commented 4 years ago

Excellent 👍

Iblis commented 4 years ago

datahub-configurations.txt Sorry for the delay, something came inbetween. I attached the datahub config as txt again. As mentioned, with this setup I am able to do mutation {deleteCar(id:209){success message}} and thereby delete the node '/Product Data/Cars/vw/Käfer/1200'

I somehow get how you want to be able to update '1200'. On the other side, being able to delete it mixes up 'path' and 'fulllpath' to me. Which also shows when using the create mutation: It won't allow you to recreate a Car on path '/Product Data/Cars/vw/Käfer/1200' anymore after the deletion of '1200'. Which is consistent to me, because you would operate on a different path. In addition, the deletion causes the Security Defintion to become invalid and being changed by data-hub, because the path it operates on does not exist anymore.

So in conclusion, I think I would rather expect that '1200' (object id 209) would not show up in listings, and you are also not allowed to update or delete it if your path is set to '/Product Data/Cars/vw/Käfer/1200'

But maybe I am just confused by now ;)

weisswurstkanone commented 4 years ago

Yes this is intendet.

Iblis commented 4 years ago

Thanks for clarification. Then we probably change our hierachy setup and introduce additional folders. This would still allow us to inherit data from parents, but won't allow read/update/delete requests on the parent.