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

File.publicUrl() unexpectedly escapes forward slashes in > 7.6.0 #2419

Open
Jeanno opened this issue Mar 3, 2024 · 11 comments
Open

File.publicUrl() unexpectedly escapes forward slashes in > 7.6.0 #2419

Jeanno opened this issue Mar 3, 2024 · 11 comments
Assignees
Labels
api: storage Issues related to the googleapis/nodejs-storage API. next major: breaking change this is a change that we should wait to bundle into the next major version type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@Jeanno
Copy link

Jeanno commented Mar 3, 2024

Summary

Forward slashes are escaped to "%2F" in File.publicUrl() for files that are > 1 level deep.

Unexpected result: "https://storage.googleapis.com/bucketId/path%2Fto%2Ffile"
Expected result: "https://storage.googleapis.com/bucketId/path/to/file"

The expected result is given before <= 6.5.4. So this issue looks like a regression.

Environment details

  • @google-cloud/storage version: > 7.6.0

Steps to reproduce

  1. Run the example code. This can also be reproduced in npm's runkit. (https://npm.runkit.com/%40google-cloud%2Fstorage)
var Storage = require("@google-cloud/storage")

var storage = new Storage.Storage();
var bucket = storage.bucket('bucketId');
var file = bucket.file('path/to/file');
var url = file.publicUrl();

Unexpected result: "https://storage.googleapis.com/bucketId/path%2Fto%2Ffile"
Expected result: "https://storage.googleapis.com/bucketId/path/to/file"

Additional info

There was an identical bug in the python library.
googleapis/google-cloud-python#3809

@Jeanno Jeanno added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Mar 3, 2024
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/nodejs-storage API. label Mar 3, 2024
@ddelgrosso1 ddelgrosso1 self-assigned this Mar 4, 2024
@ddelgrosso1
Copy link
Contributor

ddelgrosso1 commented Mar 4, 2024

Thanks for reporting this issue @Jeanno. This code path hasn't changed in some time so I am wondering if perhaps something changed on the node side. Did you recently change versions? Can you provide me with the version you are currently using?

@ddelgrosso1 ddelgrosso1 added the needs more info This issue needs more information from the customer to proceed. label Mar 6, 2024
@h-auzian
Copy link

h-auzian commented Mar 6, 2024

I can confirm that I have the same problem on my end using Node 20.

I noticed that the last version without the problem is 5.18.2. From 5.18.3 onwards, the forward slashes / are shown as %2F.

== 5.18.2: https://storage.googleapis.com/my-bucket/my-folder/my-file.csv
>= 5.18.3: https://storage.googleapis.com/my-bucket/my-folder%2Fmy-file.csv

@Jeanno
Copy link
Author

Jeanno commented Mar 7, 2024

Thanks for reporting this issue @Jeanno. This code path hasn't changed in some time so I am wondering if perhaps something changed on the node side. Did you recently change versions? Can you provide me with the version you are currently using?

I didn't change my node version (for at least a few months). The issue broken my production which I had to roll back immediately.

My development node version: node 21
Production node version: node 20. Docker image is node:20-slim to be exact.

@ddelgrosso1
Copy link
Contributor

I can confirm that I have the same problem on my end using Node 20.

I noticed that the last version without the problem is 5.18.2. From 5.18.3 onwards, the forward slashes / are shown as %2F.

== 5.18.2: https://storage.googleapis.com/my-bucket/my-folder/my-file.csv >= 5.18.3: https://storage.googleapis.com/my-bucket/my-folder%2Fmy-file.csv

Excellent thank you @HAuzian this will help me track down what happened.

@ddelgrosso1
Copy link
Contributor

See my original note #1824 (comment). Is this encoding causing an issue or is it just a readability problem as noted in this issue?

@h-auzian
Copy link

h-auzian commented Mar 7, 2024

@ddelgrosso1, thanks for linking the note providing context. The change makes perfect sense.

At least in my case, the result from File.publicUrl() was .split("/") for further processing, and that was the part that broke, but it seems to be an easy fix on my end 👍

@Jeanno
Copy link
Author

Jeanno commented Mar 8, 2024

See my original note #1824 (comment). Is this encoding causing an issue or is it just a readability problem as noted in this issue?

It took down my services in production as URLs from GCS are split by "/". I wrote my workaround so it's fine for me for now.

I'm sure there will be a lot of similar cases out there so when they upgrade their dependencies it will again cause some outages.

@ddelgrosso1
Copy link
Contributor

Being as this appears to be working as intended. Going to close this issue out. If there are any other questions or concerns, please feel free to reopen / open a new issue.

@Jeanno
Copy link
Author

Jeanno commented Mar 12, 2024

Hi @ddelgrosso1 thanks for taking a look at this. But I still think the resolution contradicts with the object naming guideline that you referenced to in the other issue.

For example, if you create an object named folder1/file.txt in the bucket your-bucket, the path to the object is your-bucket/folder1/file.txt, and Cloud Storage has no folder named folder1 stored within it. From the perspective of Cloud Storage, the string folder1/ is part of the object's name.

And specifically,

The / character [...] is typically OK to use in object names for mimicking a directory structure

On the other hand, the public urls shown on Google Cloud Console are https://storage.googleapis.com/BUCKET_ID/path/to/file instead of having encoded slashes.

Given the above, I think the change does not make sense to me. Perhaps I'm missing something here but would appreciate if someone could point that out for me.

@ddelgrosso1 ddelgrosso1 reopened this Mar 13, 2024
@ddelgrosso1
Copy link
Contributor

Let me take another look and see if I can improve on what was done.

@ddelgrosso1
Copy link
Contributor

Just some notes from poking around at this a bit, as noted the library currently encodes / which it should not be doing. However, / isn't a valid value in the name piece of a file (NOT the pathing to the file, i.e. you can't name a file sample/test.jpg). We currently leverage encodeURIComponent on the entire name which includes the folder / pathing information.

To fix this properly we may need to split the name based on pathing characters, encode the individual pieces (folders / paths can contain encodable characters), and rejoin the encoded pieces using the pathing character. I also need to check if we would want to hold off on this until the next major version update as it would likely be breaking (although one could argue it is a bug fix).

Will update accordingly.

@ddelgrosso1 ddelgrosso1 removed the needs more info This issue needs more information from the customer to proceed. label May 14, 2024
@ddelgrosso1 ddelgrosso1 added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. next major: breaking change this is a change that we should wait to bundle into the next major version and removed type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/nodejs-storage API. next major: breaking change this is a change that we should wait to bundle into the next major version type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

3 participants