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

FEATURE: More flexible id for NodeAggregate #5111

Open
1 task done
dfeyer opened this issue Jun 2, 2024 · 6 comments
Open
1 task done

FEATURE: More flexible id for NodeAggregate #5111

dfeyer opened this issue Jun 2, 2024 · 6 comments

Comments

@dfeyer
Copy link
Contributor

dfeyer commented Jun 2, 2024

Is there an existing issue for this topic?

  • I have searched the existing issues

Description

This issue is a follow-up on https://discuss.neos.io/t/rfc-non-uuid-node-identifiers/1997/16

It should be possible to use a more flexible ID for nodes. In many cases, Neos collaborate with external systems, which requires data synchronization. Being able to use arbitrary IDs can help a lot with this process.

Possible Solution

An acceptable solution can be to loosen a bit the current regexp used in NodeAggregateId with the following one:

/^[a-zA-Z0-9-_.]{1,64}$/

  • the limit of 64 can be discussed regarding index performance
  • capital letters allow the support of base58 or nanoid
  • dot and underscore are pretty common, by ex. ContentFull
@dfeyer dfeyer added the Feature label Jun 2, 2024
@dfeyer
Copy link
Contributor Author

dfeyer commented Jun 2, 2024

a direct use case of this can be the ContentRepositoryImporter if I don't have to maintain the mapping between external ids and the node ids it can make the package more simple and I can replace the mapping by a simple id prefixer strategy to avoid collisions

@bwaidelich bwaidelich added the 9.0 label Jun 2, 2024
@bwaidelich
Copy link
Member

bwaidelich commented Jun 2, 2024

I agree that we can relax the constraints even further, but this can have unwanted effects to performance (DB schema) and rendering (e.g. if we allow "<" it can suddenly pose a new XSS thread). So we should carefully think about this.

When working with external services that have really really weird IDs, you can always use a hash (and store the original id in a node property if you need the reverse direction).

/^[a-zA-Z0-9-_.]{1,64}$/

I think that's a range that we can live with – however, allowing lower and upper case characters increases the risk of errors slightly

Note: We must not forget to adjust the length (and potentially type) of the database schema if we change that

@kitsunet
Copy link
Member

kitsunet commented Jun 2, 2024

What @bwaidelich said. I am hesitant to increase size due to DB indices and schema BUT I could live with 64 I guess.

@mhsdesign
Copy link
Member

For reference the current pattern is ^([a-z0-9\-]{1,64})$ so in terms of length nothing changes.
But i see that youll proposed support for _ . and uppercase A-Z

@bwaidelich
Copy link
Member

BTW: Allowing the underscore to be part of the aggregate id would break our current implementation of CacheTags (because we use an underscore to separate the cache parts).

I suggest to change that (see #5122 (comment)) but as of now that is the case

@mhsdesign
Copy link
Member

Fyi we would also need to adjust \Neos\Neos\Fusion\ConvertUrisImplementation::PATTERN_SUPPORTED_URIS and \Neos\Neos\Service\LinkingService::PATTERN_SUPPORTED_URIS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants