refactor: generate new unique name until unknown

[danielpeintner]

I changed

• the suffix from "_2" to "_" since a third time it is no longer "2"
• all usages of Helpers.generateUniqueName(...) to use the same pattern

@derwehr

fixes #1351

[derwehr]

Thank you for addressing this issue.
I think that changing

• the suffix from "_2" to "_" since a third time it is no longer "2"

Is going to keep appending _ to generate unique names, like foo_, foo__. foo___, ...

Without it, the if body should change _2 to _3 and _3 to _4 etc.

[relu91]

I agree with @derwehr , another thing that bugs me a little bit is the function name. It does not really generate unique names but rather append numbers at the end of the title (if the generated string was really unique then we wouldn't need the while loop).

Regarding the while loop, isn't better to have a sort of escape hatch?

[danielpeintner]

Is going to keep appending _ to generate unique names, like foo_, foo__. foo___, ...

Without it, the if body should change _2 to _3 and _3 to _4 etc.

It is correct that is keeps adding _.
The problem is that if we keep _2 its gets nasty since one would need to detect that it was from a previous call. However, this is not clear since the urlPath could already be simple "blaa_2" when checked the first time etc. Hence, I think it is safer (and much simpler) with just adding until we get no more clash.

[danielpeintner]

I agree with @derwehr , another thing that bugs me a little bit is the function name. It does not really generate unique names but rather append numbers at the end of the title (if the generated string was really unique then we wouldn't need the while loop).

Correct, the name is misleading. What it does is simply adding stuff and the caller can call it as often needed to finally get a unique name. The uniqueness needs to be checked by the caller. The function doesn't know what is known already,

Does it help to rename the function to something like

Helpers.generateUniqueName(...)
Helpers.addSuffix(...)

Regarding the while loop, isn't better to have a sort of escape hatch?

Mhh, not sure if this is needed. The caller could is in charge. We could add a limit, but honestly it should not matter.. I think..

[JKRhb]

It does not really generate unique names but rather append numbers at the end of the title (if the generated string was really unique then we wouldn't need the while loop).

Hmm, how about using a timestamp, a UUID, or just some random string instead?

[relu91]

The problem is that if we keep _2 its gets nasty since one would need to detect that it was from a previous call. However, this is not clear since the urlPath could already be simple "blaa_2" when checked the first time etc. Hence, I think it is safer (and much simpler) with just adding until we get no more clash.

Shouldn't generate _3 in the next call? given that it is detecting if was already slugfied with this regex?

Hmm, how about using a timestamp, a UUID, or just some random string instead?

It would be an option but then I would rather start thinking about properly using the thing id (since it was not used because less human-readable than then title).

[danielpeintner]

Not sure now how we proceed best. Let me summarize my thinking.

• In most of the cases, the title should already lead to a unique name. What we currently look for is the (hopefully rare) situation where there are clashing titles.
• The method Helpers.generateUniqueName(...) does not really do what it should and is not really a useful method as is.

All this makes me think that we might want to remove the helper method and each binding can take care. If it were me, I would
simply change the implementations as follows

<<<< Now

        let urlPath = slugify(thing.title, { lower: true });
        while (this.things.has(urlPath)) {
            urlPath = Helpers.generateUniqueName(urlPath);
        }

>>>> Afterwards

        let urlPath = slugify(thing.title, { lower: true });
        while (this.things.has(urlPath)) {
            urlPath = urlPath + "_"; // other suffixes possible also
        }

No need for this method at all. Thoughts?

[danielpeintner]

@egekorkan mentioned that yet another possibility would be to refuse such TDs to be loaded/served.

@RobWin Out of curiosity, do you have the same situation? What happens if the runtime is asked to serve TDs that use the same title in their TDs. We use the title to create human-readable URLs. The general question is what to do best when such clashes happen...

[relu91]

As discussed internally, I think the short-term goal is to solve #1351 . The current changes sufficiently cover the use case, in the future we will explore an id based paths.

[danielpeintner]

@relu91 I agree this PR solves the problem, but I don't really like the solution 🤷‍♂️. I created #1351 to show how I would like to move forward. What do you and others think?

[danielpeintner]

Closed in favor of #1375