Skip to content

Conversation

@danielpeintner
Copy link
Member

@danielpeintner danielpeintner commented Feb 18, 2025

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

error  'paths' is never reassigned. Use 'const' instead  prefer-const
@derwehr
Copy link
Contributor

derwehr commented Feb 18, 2025

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
Copy link
Member

relu91 commented Feb 18, 2025

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
Copy link
Member Author

danielpeintner commented Feb 18, 2025

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
Copy link
Member Author

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
Copy link
Member

JKRhb commented Feb 18, 2025

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
Copy link
Member

relu91 commented Feb 18, 2025

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
Copy link
Member Author

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
Copy link
Member Author

@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...

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

@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
Copy link
Member Author

Closed in favor of #1375

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exposing more than 2 instances of the same thing generates duplicate URIs

4 participants