neos / neos-development-collection

The unified repository containing the Neos core packages, used for Neos development.
https://www.neos.io/
GNU General Public License v3.0
260 stars 220 forks source link

FEATURE: More flexible id for NodeAggregate #5111

Open dfeyer opened 1 month ago

dfeyer commented 1 month ago

Is there an existing issue for this topic?

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}$/

dfeyer commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

mhsdesign commented 1 month ago

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 commented 1 month ago

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 https://github.com/neos/neos-development-collection/pull/5122#discussion_r1625442310) but as of now that is the case

mhsdesign commented 2 days ago

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