Wrap net/url to fix URI handling by sdassow · Pull Request #6357 · fyne-io/fyne

sdassow · 2026-06-10T11:58:01Z

Description:

Handle URIs with the standard library. URNs are supported by detecting the scheme and passing through the path.

Checklist:

Tests included.
Lint and formatter run with no errors.
Tests all pass.

…ding

…turning string

coveralls · 2026-06-10T12:42:17Z

coverage: 60.014% (-0.1%) from 60.155% — sdassow:fix/file-uri-escaping into fyne-io:develop

MaxGyver83 · 2026-06-10T13:12:24Z

This implementation uses a statically generated table for performance reasons.

Is this much faster than escaped := url.URL{Scheme: u.scheme, Path: u.path}? Does this speed-up really matter in a real app?

andydotxyz · 2026-06-10T13:28:13Z

+	false, // 0x20
+	false, // 0x21
+	false, // 0x22
+	false, // 0x23


Printable characters on a lookup table (like #) would be much more helpful than the hex code

toaster

As discussed in Slack, I would prefer to use the stdlib’s URL instead of re-inventing the wheel here.

I’d also like to get another opinion on this topic.

For the actual implementation:

You should explicitly test all the bytes and maybe a couple of unicode characters.

Optional:

I suggest to use a shouldEscape[c] instead of the dontEscape[c] approach.
You might simplify the generator implementation.

toaster · 2026-06-10T12:45:59Z

+func filePathEscape(path string) string {
+	length := len(path)
+	for _, c := range []byte(path) {
+		if !dontEscape[c] {


I think an inverted list would be easier to read here and not worse later:

if escape[c] { … } … if escape[c] { /* escape */ } else { … }

toaster · 2026-06-10T12:48:24Z

+	assert.Equal(t, "/home/user/file.txt", filePathEscape("/home/user/file.txt"))
+	assert.Equal(t, "/home/user/file%231.txt", filePathEscape("/home/user/file#1.txt"))
+}


This test is not sufficient. You can remove characters from the list to escape without breaking the test.

toaster · 2026-06-10T12:51:31Z

+	dontEscape['$'] = true
+	dontEscape['&'] = true
+	dontEscape['+'] = true
+	dontEscape['-'] = true
+	dontEscape['.'] = true
+	dontEscape[':'] = true
+	dontEscape['='] = true
+	dontEscape['@'] = true
+	dontEscape['_'] = true
+	dontEscape['~'] = true
+
+	dontEscape['\\'] = true
+	dontEscape['/'] = true


for _, rune in range "$&+-.:=@_~\\/" { dontEscape[rune] = true }

sdassow · 2026-06-11T09:11:57Z

As discussed in Slack, I would prefer to use the stdlib’s URL instead of re-inventing the wheel here.

[snip]

Just for reference, I looked into this, and we were using net/url for parsing until d95d323, but still maintain our own implementation otherwise, and we could be using url.URL instead, which I think is what you're suggesting.

I'll try and see how far I get before spending more time on the details of the current PR, but the feedback is good and appreciated and will be applied if it turns out it's still needed.

sdassow · 2026-06-11T09:26:01Z

This implementation uses a statically generated table for performance reasons.

Is this much faster than escaped := url.URL{Scheme: u.scheme, Path: u.path}? Does this speed-up really matter in a real app?

The (probable (since I've done this before)) speed-up probably won't make a real-world difference in a Fyne app and is more a side-effect than the goal, and the comment was to explain why this particular implementation (static lookup table, no function call overhead).

I'm now looking if we could wrap url.URL instead like Tilo suggested.

toaster

Cool, just some minor things:

consider type uri url.URL (constructed via uri{…})
use direct prop access

toaster · 2026-06-11T11:27:09Z

 type uri struct {
-	scheme    string
-	authority string
-	path      string
-	query     string
-	fragment  string
+	url.URL
 }


Why not type uri url.URL?

Public fields of url.URL conflict with the accessor names/API unfortunately.

toaster · 2026-06-11T11:29:58Z


 func (u *uri) Extension() string {
-	return path.Ext(u.path)
+	return path.Ext(u.URL.Path)


Whether you actually embed or simply do type uri url.URL does not matter, this is accessible as u.Path in both cases.

This applies to all other u.URL.*.

I've changed the few that don't actually need it, but this one does due to the Path method/field conflict.

…(possible?) problems

…m about (possible?) problems" This reverts commit 42952fe.

…evert test to previous behaviour

… says 'must remove ...'

toaster

someone else should check the decisions regarding urn and Windows file URI in ParseURI
check whether uri#String() can simply be removed
add exceptional cases to getUserHost test
improve comprehensibility of hostname regexp by extracting host component pattern into a constant, i.e., by giving it a name

toaster · 2026-06-24T11:21:58Z

+		{"foo@bar", "foo", "bar"},
+		{"@bar", "", "bar"},
+		{"foo:bar@baz", "foo:bar", "baz"},
+		{"foo:bar@", "foo:bar", ""},


You should add exceptional cases here, too, e.g. foo:bar:baz@nowhere.

Generally, I don’t like the idea of testing unexported functions because you do not guarantee the exported functions working as expected this way.

toaster · 2026-06-24T11:37:08Z

 	"fyne.io/fyne/v2"
 )

+var rxHostName = regexp.MustCompile(`^[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$`)


I think, you should extract the host name component part into a constant:

const hostNameComponentPattern = "[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?" var rxHostName = regexp.MustCompile("^" + hostNameComponentPattern + `(?:\.` + hostNameComponentPattern + ")*$")

toaster · 2026-06-24T11:39:26Z

-	}
-	return s.String()
+	return u.URL.String()
 }


Isn’t this method superfluous now?
I think removing it would provide u.URL.String() directly to uri.

…dge cases and examples with encoding

sdassow · 2026-06-25T08:45:10Z

Great feedback, thanks! All addressed.

someone else should check the decisions regarding urn and Windows file URI in ParseURI

Yes, please!

andydotxyz · 2026-06-26T22:03:35Z

If urn is called out as a special case I think it needs tests covering that...

… for query and fragment

andydotxyz

great thanks

sdassow added 2 commits June 10, 2026 13:54

Add fix and test for file URIs with special characters that need enco…

cfbcef8

…ding

Fix ranges

dbd2f41

sdassow marked this pull request as draft June 10, 2026 12:23

Unescape path when handling file URIs in ParseURI, and escape when re…

b241709

…turning string

sdassow marked this pull request as ready for review June 10, 2026 12:34

MaxGyver83 reviewed Jun 10, 2026

View reviewed changes

Comment thread storage/repository/uri.go Outdated

andydotxyz reviewed Jun 10, 2026

View reviewed changes

toaster requested changes Jun 10, 2026

View reviewed changes

Switch to net/url and handle URNs

1d370a0

sdassow changed the title ~~Add fix and test for file URIs with special characters~~ Wrap net/url to fix URI handling Jun 11, 2026

toaster requested changes Jun 11, 2026

View reviewed changes

sdassow added 15 commits June 11, 2026 15:02

Add host format checks

7db14ab

Merge branch 'develop' into fix/file-uri-escaping

965b51e

Log error trying to narrow down an issue, and generally inform about …

42952fe

…(possible?) problems

Merge branch 'develop' into fix/file-uri-escaping

29d671b

Add tests for getUserHost function

3c858c5

Clean up and clarify a bit

00a1851

Only execute further tests when Create() was successful

50c59c8

Only do Close() conditionally, so Remove() still catches failures

383bcee

Better host validation and some tests

408a926

Get rid of special case and adjust test accordingly

e73fb2d

Merge branch 'develop' into fix/file-uri-escaping

7705b42

Revert "Log error trying to narrow down an issue, and generally infor…

4d8de66

…m about (possible?) problems" This reverts commit 42952fe.

Add check for nil to please the linter

66c7805

Only check for double slash when schemes have no custom repository

abf1db4

Only do double slash check for file scheme

b07a7d0

sdassow added 12 commits June 22, 2026 12:54

Specifically check empty paths, insert double slash if missing, and r…

8a6fb0d

…evert test to previous behaviour

Fix file paths on windows

31b9ca5

Merge branch 'develop' into fix/file-uri-escaping

a707e10

Linter says 'could remove embedded field "URL" from selector', but CI…

a255e10

… says 'must remove ...'

Must. Change.

90e1553

Use ParseURI instead of deprecated NewURI and check result

670c2be

Need to convert slashes on this code-path too

7bac1ba

Always convert slashes on windows for file URIs

5c94fc9

Decode path and use NewFileURI instead of url.Parse

870979a

Clean up a bit

520550f

Add special case for windows to support URIs without file scheme

589d58c

Don't add extra slash (technically correct) on windows

cb3bdb3

sdassow requested a review from toaster June 23, 2026 11:41

Revert unnecessary change

f147fa5

sdassow requested review from MaxGyver83 and andydotxyz June 23, 2026 11:57

Tidy modules

d394004

toaster requested changes Jun 24, 2026

View reviewed changes

sdassow added 4 commits June 25, 2026 07:39

Move domain label pattern to constant

70db510

Only return user when non-empty, move tests to better file, and add e…

2f3db5b

…dge cases and examples with encoding

Use embedded String method

d87f582

Merge branch 'develop' into fix/file-uri-escaping

d5841e8

Add another test case with encoded separator in username

ef993dc

sdassow requested a review from toaster June 25, 2026 10:21

Add more URN tests, use opaque field instead of path, and add support…

cf70723

… for query and fragment

andydotxyz approved these changes Jun 27, 2026

View reviewed changes

Uh oh!

Uh oh!

Conversation

sdassow commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

coveralls commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

MaxGyver83 commented Jun 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdassow commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdassow commented Jun 11, 2026

Uh oh!

toaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdassow Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toaster left a comment • edited by sdassow Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdassow commented Jun 25, 2026

Uh oh!

andydotxyz commented Jun 26, 2026

Uh oh!

andydotxyz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sdassow commented Jun 10, 2026 •

edited

Loading

coveralls commented Jun 10, 2026 •

edited

Loading

sdassow commented Jun 11, 2026 •

edited

Loading

sdassow Jun 11, 2026 •

edited

Loading

toaster left a comment •

edited by sdassow

Loading