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

resolveuid deserializer doesn't handle lists or nested dicts #1594

Closed
JeffersonBledsoe opened this issue Mar 6, 2023 · 3 comments · Fixed by #1595
Closed

resolveuid deserializer doesn't handle lists or nested dicts #1594

JeffersonBledsoe opened this issue Mar 6, 2023 · 3 comments · Fixed by #1595

Comments

@JeffersonBledsoe
Copy link
Member

The resloveuid deserializer currently makes the assumption that the url or href field is in the root of the block data, or it has been created by a volto_object_browser that is set to single mode. This means that making use of nested or lists of block data or lists (such as with the object_list widget in Volto) doesn't cause the field to be converted to use resolveuid.

For example, the following block data:

{
  "123": {
    "@type": "foo",
    "my_values": [
      {
        "url": "/news"
      }
    ]
  }
}

Should end up being stored as this:

{
  "123": {
    "@type": "foo",
    "my_values": [
      {
        "url": "../resolveuid/{doc_uid}"
      }
    ]
  }
}

But instead is saved as in the original

@davisagli
Copy link
Sponsor Member

The current situation is that you can implement an IBlockFieldDeserializationTransformer adapter to do it for the specific blocks and fields where it is needed. For example plone.volto has one that handles fields named columns, hrefList, or slides: https://github.com/plone/plone.volto/blob/main/src/plone/volto/transforms.py#L45

The idea of handling this generically might be okay as long as:
a. the performance impact is not too bad
b. the volto team isn't concerned about the backwards incompatibility (transforming fields that were not transformed before)

I would feel better if it were based on a block schema so that we could do it only for the fields that we know are referencing object paths, rather than using a heuristic based on field name.

@JeffersonBledsoe
Copy link
Member Author

JeffersonBledsoe commented Mar 7, 2023

@davisagli Fairly new to using the block transforms so forgive me for any lack of understanding:

a. the performance impact is not too bad

This is my biggest concern with the change. There's an initial PR linked to this issue if anybody wanted to test this against their bigger blocks

b. the volto team isn't concerned about the backwards incompatibility (transforming fields that were not transformed before)

While the actual data would change, a resolveuid should still return the same URL for a working link right? Would this be breaking or not?

I would feel better if it were based on a block schema so that we could do it only for the fields that we know are referencing object paths, rather than using a heuristic based on field name.

Completely agreed. However, we don't really have the notion of a URL field right? The only hint is the widget in the frontend when using Volto. Would we need some kind of formal schema (e.g. #923 or plone/volto#2252) support for blocks to achieve this?

For more specific use cases, I agree that it's probably best to have specific adapters for more specific use cases, but I image most sites would want resolveuid rather than just storing the path.

@davisagli
Copy link
Sponsor Member

@JeffersonBledsoe I will try to take another look at this during the Beethoven Sprint. I'm not too worried about the performance impact or backwards compatibility, and it would certainly help avoid the need for lots of custom adapters.

I also have an idea about storing some information during resolveuid deserialization to be used by the linkintegrity retriever instead of walking the blocks again, which could help recuperate any loss of performance.

One important note: we need a similar change to the serializer to make sure the resolveuid URLs actually get resolved when getting blocks content.

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 a pull request may close this issue.

2 participants