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

contextual vocabularies #6236

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

contextual vocabularies #6236

wants to merge 13 commits into from

Conversation

mamico
Copy link
Member

@mamico mamico commented Aug 5, 2024

Fixes #3216

When there is a contextualized vocabulary url in the schema, Volto must call it with the full path and not return it to the site root.

There is a risk, however, that this change may bring out problems hidden by the anomaly it solves.

Copy link

netlify bot commented Aug 5, 2024

Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit 093517e
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66fea03fc7601b000805022a

@davisagli
Copy link
Member

I've run into this problem on one project, but I decided it was too risky to load all vocabularies with a context-sensitive path. For one thing, this means the results can't be HTTP-cached in cases when they are not context-dependent.

For that project, I customized this action to check a setting with a list of vocabularies that are context-dependent. That way it's possible to opt in where it's needed.

  const vocabulary = getVocabName(vocabNameOrURL);
  let vocabPath;
  if (config.settings.contextualVocabularies.includes(vocabulary)) {
    vocabPath = flattenToAppURL(vocabNameOrURL);
  } else {
    vocabPath = `/@vocabularies/${vocabulary}`;
  }

  let queryString = `b_start=${start}${size ? '&b_size=' + size : ''}`;
  if (query) {
    queryString = `${queryString}&title=${query}`;
  }
  return {
    type: GET_VOCABULARY,
    vocabulary: vocabNameOrURL,
    start,
    request: {
      op: 'get',
      path: `${vocabPath}?${queryString}`,
    },
    subrequest,
  };

@mamico
Copy link
Member Author

mamico commented Aug 5, 2024

I've run into this problem on one project, but I decided it was too risky to load all vocabularies with a context-sensitive path. For one thing, this means the results can't be HTTP-cached in cases when they are not context-dependent.

@davisagli, Good point! Do you think your solution might be in Volto?

With config.settings.contextualVocabularies defaulting to []

@davisagli
Copy link
Member

@mamico I think that would be okay, and fully backward-compatible.

On the other hand, it might be nice if there were a way to mark a vocabulary as context-dependent when it is defined, and have plone.restapi & volto use that information. There would be a lot more details and moving parts to figure out for this, though.

@mamico

This comment was marked as resolved.

@mamico mamico marked this pull request as ready for review August 14, 2024 11:24
@mamico mamico requested review from davisagli and pnicolli August 14, 2024 11:29
@wesleybl wesleybl modified the milestone: 18.x.x Aug 19, 2024
Copy link
Member

@sneridagh sneridagh left a comment

Choose a reason for hiding this comment

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

@mamico @davisagli

Maintaining the list might suck, and not obvious for front-end/integrators that do not know about which vocabularies are contextual and which not.

Can we provide a default list of vocabularies at least? or defaulting to [] is for backwards compatibilities concerns? Maybe suggest a good known list of them for new projects? or the other way around, suggest the list by default, warn about it in the upgrade guide and suggest people to use [] if they want to be conservative.

Aside of this, LGTM.

@mamico
Copy link
Member Author

mamico commented Oct 3, 2024

@mamico @davisagli

Maintaining the list might suck, and not obvious for front-end/integrators that do not know about which vocabularies are contextual and which not.

Agree

Can we provide a default list of vocabularies at least? or defaulting to [] is for backwards compatibilities concerns? Maybe suggest a good known list of them for new projects? o

We can look into the default vocabularies of plone.restapi, but if we set a default other than [], this change will be breaking. Is this acceptable for version 18.* ?

or the other way around, suggest the list by default, warn about it in the upgrade guide and suggest people to use [] if they want to be conservative.

I had set the needs: docs label from the beginning for that purpose, just needed time to do it. :)

@davisagli
Copy link
Member

@mamico @sneridagh So a different and maybe easier to use approach here would be:

  1. If a vocab name is passed, get it from the root (same as now)
  2. If a vocab URL is passed, get it from the URL as written, instead of extracting the name and fetching it from the root. This is a breaking change, it makes these vocabularies contextual.
  3. Make sure that when plone.restapi generates a vocabulary URL, it uses an appropriate context. This might require inventing a way to mark on the vocabulary factory whether it is context-dependent or not.

We need to keep in mind that there might be a core vocabulary which never varies based on the context, but an integrator needs to replace it with a custom version that is context-dependent.

@mamico
Copy link
Member Author

mamico commented Oct 7, 2024

@davisagli @sneridagh

We need to keep in mind that there might be a core vocabulary which never varies based on the context, but an integrator needs to replace it with a custom version that is context-dependent.

IMHO we are talking about acting in the serializer of schemes, like here:

https://github.com/plone/plone.restapi/blob/f68fab7ea3209a096423d8567fa7f70a93786c7a/src/plone/restapi/types/adapters.py#L110

in other cases the developer has complete control of the vocabulary url.

My plan:

  1. We change the serializer to return the full vocabulary url for contextual vocabularies, and only the vocabulary_name (or the vocabulary url pointing to the portal root?) for non-contextual vocabularies.

  2. change (like my original proposal) to use the vocabulary url as is, if it is a url, or a vocabulary url, based on the portal root, if it is a vocabulary name.

I think both changes could be independent, without major breaking, and there are no needs for a new configuration.

thoughts?

@pnicolli
Copy link
Contributor

@davisagli @mamico this is an interesting PR, how can we keep this going?

@davisagli
Copy link
Member

@pnicolli @mamico Sorry, I'll come back to this, but I've been busy getting ready for the conference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Work approved
Development

Successfully merging this pull request may close these issues.

Fix for getting a vocabulary that depends on the context
5 participants