Skip to content

Conversation

usu
Copy link
Member

@usu usu commented Nov 21, 2021

This is needed because columns property on ColumnLayout now comes as a JSON array, which is treated as embedded collection by mistake. Previously the information came in a JSON object (jsonConfig property).

There's an edge case which would now produce an error: If an empty embedded collection is provided, there's currently no chance this would be identified as an embedded collection, as this information is already taken away by the normalizer. This probably throws a console error, because the developer would expect a function (embedded collection) but would receive an empty array.

Shouldn't be a problem for us at the moment, because we always provide links for embedded collections, in which case the empty array is already replaced by the normalizer by the link and everything works fine.

@usu usu requested a review from carlobeltrame November 21, 2021 15:11
Copy link
Member

@carlobeltrame carlobeltrame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! In the long run, I'd like to think about ways to circumvent the empty array problem. Maybe the normalizer should (optionally) add a fake link, or we should keep track of the pre-normalization embedded entities. But for now, this fixes 90% of the problem.

@carlobeltrame carlobeltrame merged commit bacc91d into ecamp:master Nov 22, 2021
@carlobeltrame
Copy link
Member

hal-json-vuex 2.0.0-alpha.10 was just published, including this fix.

@BacLuc
Copy link
Contributor

BacLuc commented Nov 22, 2021

Should that not be merged into the next branch?

@carlobeltrame
Copy link
Member

Ooops, you're right. @usu I can't simply apply the same commit to next because of typescript. Do you have time to redo the fix for typescript or should I have a look?

@usu
Copy link
Member Author

usu commented Nov 22, 2021

Oh yes, true. I can prepare it for the next branch.

@usu
Copy link
Member Author

usu commented Nov 23, 2021

Thanks! In the long run, I'd like to think about ways to circumvent the empty array problem. Maybe the normalizer should (optionally) add a fake link, or we should keep track of the pre-normalization embedded entities. But for now, this fixes 90% of the problem.

We need to fix it in the short run :-)
The problem is not only with empty arrays, but also if the collection is provided as an array of links, e.g. like below for property preferredContentTypes. We have this currently at several places within the new eCamp API.

I like the idea with the fake link. The embedded collection could be stored under "/categories/ed50d2c7744b.preferredContentTypes". Or by using a hash to avoid any potential conflict with real URIs.

{
  "_links": {
    "self": {
      "href": "/categories/ed50d2c7744b"
    },
    "preferredContentTypes": [
      {
        "href": "/content_types/d10a9b214b88"
      }
    ],
  },
  "_embedded": {
    "preferredContentTypes": [
      {
        "_links": {
          "self": {
            "href": "/content_types/d10a9b214b88"
          },
          "contentNodes": {
            "href": "/content_node/single_texts?contentType=%2Fcontent_types%2Fd10a9b214b88"
          }
        },
        "name": "SafetyConcept",
        "active": true,
        "id": "d10a9b214b88"
      }
    ],
  },
  "short": "LA",
  "name": "Lageraktivität",
  "color": "#FF9800",
  "numberingStyle": "1",
  "id": "ed50d2c7744b"
}

@carlobeltrame
Copy link
Member

Not sure I get the problem here, wouldn't the normalizer output the following for your example?

{
  "/categories/ed50d2c7744b": {
    _meta: { self: { href: "/categories/ed50d2c7744b" } },
    preferredContentTypes: [{
      "href": "/content_types/d10a9b214b88"
    }],
    short: 'LA',
    // other primitive properties
  },
  "/content_types/d10a9b214b88": {
    _meta: { self: { href: "/content_types/d10a9b214b88" } },
    contentNodes: { href: "/content_node/single_texts?contentType=%2Fcontent_types%2Fd10a9b214b88" },
    name: "SafetyConcept",
    // other primitive properties
  }
}

And our check for the first entry in preferredContentTypes would detect { "href": "/content_types/d10a9b214b88" } to be an entity reference..?
Of course, the problem still happens when a category has no preferredContentTypes.

@usu
Copy link
Member Author

usu commented Nov 24, 2021

Of course, the problem still happens when a category has no preferredContentTypes.

Yes sorry, my bad. The problem only occurs for empty arrays.

At first I believed it's not an issue for us, as API platform always provides the links to all relations. But I forgot that these link properties could also be arrays and therefore be empty, too.

Example of working example (campCollaborations in camp):

{
  "_links": {
    "self": {
      "href": "/camps/b91a1c5c8e85"
    },
    "campCollaborations": {
      "href": "/camp_collaborations?camp=/camps/b91a1c5c8e85"
    },
  },
  "_embedded": {
    "campCollaborations": []
  },
  "id": "b91a1c5c8e85"
}

Example with error (empty link array):

{
  "_links": {
    "self": {
      "href": "/categories/243b3b0d2597"
    },
    "preferredContentTypes": [],
  },
  "_embedded": {
    "preferredContentTypes": []
  },
  "id": "243b3b0d2597"
}

@carlobeltrame
Copy link
Member

carlobeltrame commented Nov 24, 2021

But that means the immediate problem would be solved for our case if we added the ability to "filter (preferred)ContentTypes by category". Deep filters such as /content_nodes?owner.category=/categories/1a2b3c4d or in this case /content_types?preferredIn.category=/categories/1a2b3c4d are supported by API Platform out of the box (if you add the doctrine relation and the #[ApiFilter]). Once the filter is in place, we can make sure there is a single filtered link from category to preferredContentTypes, instead of an array of links. If the link isn't inserted automatically based on the doctrine relation and filter, you might need to add a #[RelatedCollectionLink] annotation.

@usu
Copy link
Member Author

usu commented Nov 24, 2021

Yes, I can try. We have a few non-trivial edge cases, need to check how well it works:

  • category.preferredContentTypes --> contentType has not relation back to category (unidrectional relationship by purpose)
  • materialNode.materialItems --> works as expected if loaded via /material_node endpoint. Doesn't work if loaded via generic /content_node endpoint.
  • activity.campCollaborations --> manually resolved via ActivityResponsible

@carlobeltrame
Copy link
Member

👍 let me know if I can help you with that

@usu
Copy link
Member Author

usu commented Nov 25, 2021

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.

3 participants