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

Use documentId as the unique identifier #65

Open
jnachtigall opened this issue Aug 30, 2022 · 5 comments
Open

Use documentId as the unique identifier #65

jnachtigall opened this issue Aug 30, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@jnachtigall
Copy link

jnachtigall commented Aug 30, 2022

We have a custom type, that only has a uniq headline (in german) and a text field

image

This gets exported as

      customTypes: [
        {
          configName: "article",
          queryString: "api::article.article",
          uid: ["headline"],
        },
    ]

On the filesystem you then have files like

strapi-app/config/sync$ ls -lh article.*
-rw-r--r-- 1 jnachtigall jnachtigall 517 Aug 30 16:44 'article.Benötige ich eine wasserrechtliche Erlaubnis?.json'
-rw-r--r-- 1 jnachtigall jnachtigall 575 Aug 30 16:44 'article.Gibt es Fälle, in denen ich keine Anzeige stellen kann?.json'
-rw-r--r-- 1 jnachtigall jnachtigall 436 Aug 30 16:44 'article.Wie kann ich mich authentifizieren $ anmelden?.json'

This works fine on Linux or in WSL2. However, on Windows this results in these git errors

$ git -c credential.helper= -c core.quotepath=false -c log.showSignature=false rebase origin/develop
error: invalid path 'strapi-app/config/sync/article.Benötige ich eine wasserrechtliche Erlaubnis?.json'
error: invalid path 'strapi-app/config/sync/article.Gibt es Fälle, in denen ich keine Anzeige stellen kann?.json'
error: invalid path 'strapi-app/config/sync/article.Wie kann ich mich authentifizieren $ anmelden?.json'
error: could not detach HEAD

This seems like a pretty well known error, see https://stackoverflow.com/questions/63727594/github-git-checkout-returns-error-invalid-path-on-windows

FWIW, doing the suggested git config core.protectNTFS false did not work for us (resulted in read/write errors and a bluescreen).

We worked around the issue by adding a field articleId with integer to the above custom type and used this as uid instead.

Not sure you want to fix this in your plugin, but maybe a encoding/decoding of non-ascii characters would be good in the filename?

System

  • Node.js version: 16.13.0
  • NPM version: 8.3.1
  • Strapi version: 4.3.0
  • Plugin version: 1.0.2
  • Database: Postgres Docker image
  • Operating system: WSL2 in Windows
@boazpoolman boazpoolman added the bug Something isn't working label Aug 30, 2022
@boazpoolman
Copy link
Member

Allright. Thanks for the bug report.
You’re kinda hitting the current limit of the uid feature.

The uid will be used in the filename and therefor cannot contain characters which are not supported in filenames on your OS.
I’ve allready escaped two characters (semicolon and forward slash) see issue #14 and #56. Though because you’re using a title like field as the uid there are many characters that are likely not supported in a filename.

As you suggested we need to encode the uid value to be a valid filename, and we also need to be able to decode that filename back to it’s original state. Otherwise we can’t match the uid value in the DB.

I’m not planning on fixing this any time soon, but I’ll leave it open for reference. In your case it is best to use another field as the uid. You could create an extra field of type uid which encodes the headline field and saves that in the DB in a seperate field. Than that seperate field can be the uid for your custom config type.

@boazpoolman boazpoolman added the wontfix This will not be worked on label Sep 23, 2022
@boazpoolman
Copy link
Member

boazpoolman commented Jul 27, 2023

We might have to consider dropping the uid system as it is, and switch to a uuid approach.
That would solve this and a lot of other issues.

@boazpoolman
Copy link
Member

I'm rebranding this issue from a bug to an enhancement.

I would like to see a new major version of the plugin with a proper uuid system.
As of writing this comment the config types are required to have a unique field that is used to match entries across databases. Though that is very prone to errors and is why this issue (and a few others) were created.

I would suggest we inject a uuid field onto the config types in the register function of this plugin, or we use some 3rd party plugin to do that for us. Then that value can be used to write the JSON filename, and be used as a unique identifier.

@boazpoolman boazpoolman added enhancement New feature or request and removed bug Something isn't working wontfix This will not be worked on labels Sep 11, 2023
@boazpoolman boazpoolman changed the title custom type with uid with non-ascii chars gives error: invalid path on Windows Use generated uuid as the unique identifier Sep 11, 2023
@boazpoolman
Copy link
Member

With Strapi 5 being stable, we might be able to use it's CUID documentId as the unique identifier of config types.
I'll do some testing to see if this can be added to v2 of the config sync plugin

@boazpoolman boazpoolman added this to the v2.0.0 milestone Sep 24, 2024
@boazpoolman
Copy link
Member

I just realised that by using the document ID as the unique identifier we lose the ability to exclude certain configs by default. Those configs are excluded here and are referenced by their names. If we were to use the document ID, that would mean that the names will be different for every instance using this plugin. Losing the ability to exclude these by default.

Soo.. I'll likely stick to the uid system as it is.

Something that we could do is make the uid optional and if it's left empty fall back to the document ID. That way custom types can be registered without a uid, but the default types will still have the custom uid to exclude some configs by default. Though I don't have much incentive to build this as I believe it's quite a niche feature, and I'm not sure how many people use the custom types feature anyways.

@boazpoolman boazpoolman removed this from the v2.0.0 milestone Oct 12, 2024
@boazpoolman boazpoolman changed the title Use generated uuid as the unique identifier Use generated documentId as the unique identifier Oct 12, 2024
@boazpoolman boazpoolman changed the title Use generated documentId as the unique identifier Use documentId as the unique identifier Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants