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

Fix truncating urls for paths with multiple dots (#46) #45

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gpoole
Copy link

@gpoole gpoole commented Nov 28, 2022

Fix for public IDs containing multiple dots before the extension getting truncated. For example, in 0.2.4 https://res.cloudinary.com/xyz/image/upload/v111111111/Folder/Name.With.Multiple.Dots.png is extracted as Folder/Name instead of Folder/Name.With.Multiple.Dots. This fix (maybe incorrectly) assumes everything after the last . is an extension, but it does correctly include any dots before the last . in the ID so it should work for most cases.

Not sure if it should be more careful about specifically only removing image and video extensions but I thought that might be over-complicating it.

Fixes #46

@gpoole gpoole changed the title Fix truncating urls for paths with multiple dots (#22) Fix truncating urls for paths with multiple dots (#46) Nov 28, 2022
@gpoole
Copy link
Author

gpoole commented Nov 28, 2022

Sorry for messing around with the related issue, I thought it was related to #22 but I think actually it's a slightly different problem after re-reading the original description closely.

@orimdominic
Copy link

Nice solution @gpoole
What do you think about this one though?

@mayashavin
Copy link
Owner

@colbyfayock can you take a look on this?

@gpoole
Copy link
Author

gpoole commented Feb 15, 2023

Sorry, I missed that @orimdominic. That seems reasonable and probably more foolproof, although I think the path.extname part won't work in browsers. The big regex could be replaced with that split though which seems a lot simpler. I'll take a look if I get time.

@orimdominic
Copy link

Nice to know @gpoole
I was working with Node.js when I developed that solution. I had no idea this library would be used in the browser too

@colbyfayock
Copy link
Collaborator

im not great at regex, but one thing to consider is the extension isn't required

  • .../myimage.png
  • .../myimage

@gpoole
Copy link
Author

gpoole commented Feb 21, 2023

@colbyfayock as the extension is removed with a .replace, it will only remove one if found but if there is no extension nothing will happen. I've added a test validating that's the case.

@orimdominic I had a go at the solution you suggested but unfortunately it doesn't quite work, since the extracted ID with that method includes the v12345/ part of the path, which caused tests to fail. I was going to try stripping the v12345 bit, but when I was looking at the existing regex again I noticed that it also handles a lot of variants of the URL format, such as the path including private/ or youtube/ instead of upload/, so I think it's probably safer and simpler to keep it as is.

@orimdominic
Copy link

@colbyfayock as the extension is removed with a .replace, it will only remove one if found but if there is no extension nothing will happen. I've added a test validating that's the case.

@orimdominic I had a go at the solution you suggested but unfortunately it doesn't quite work, since the extracted ID with that method includes the v12345/ part of the path, which caused tests to fail. I was going to try stripping the v12345 bit, but when I was looking at the existing regex again I noticed that it also handles a lot of variants of the URL format, such as the path including private/ or youtube/ instead of upload/, so I think it's probably safer and simpler to keep it as is.

This is great! 👍

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.

URLs containing multiple dots before the extension get truncated when building URLs
4 participants