fix(gnoweb): simplify url parsing system #3366

gfanton · 2024-12-18T14:07:46Z

This PR simplifies the URL parsing system:

Removes the complex regexp that was introduced in the gnoweb revamp.
Adds a bunch of new parsing tests for GnoURL.
Makes the Encode method more composable to adapt to various use cases.

Signed-off-by: gfanton <[email protected]>

Gno2D2 · 2024-12-18T14:09:48Z

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):

IGNORE the bot requirements for this PR (force green CI check)

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)

☑️ Contributor Actions:

Fix any issues flagged by automated checks.
Follow the Contributor Checklist to ensure your PR is ready for review.
- Add new tests, or document why they are unnecessary.
- Provide clear examples/screenshots, if necessary.
- Update documentation, if required.
- Ensure no breaking changes, or include BREAKING CHANGE notes.
- Link related issues/PRs, where applicable.

☑️ Reviewer Actions:

Complete manual checks for the PR, including the guidelines and additional checks if applicable.

📚 Resources:

Debug

Automated Checks
Maintainers must be able to edit this pull request (more info)
If
🟢 Condition met
└── 🟢 The pull request was created from a fork (head branch repo: gfanton/gno)
Then
🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request
Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)
If
🟢 Condition met
└── 🟢 On every pull request
Can be checked by

Any user with comment edit permission

Signed-off-by: gfanton <[email protected]>

codecov · 2024-12-18T14:48:57Z

Codecov Report

Attention: Patch coverage is 91.30435% with 8 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
gno.land/pkg/gnoweb/url.go	91.13%	5 Missing and 2 partials ⚠️
gno.land/pkg/gnoweb/handler.go	92.30%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Signed-off-by: gfanton <[email protected]>

alexiscolin · 2024-12-19T09:40:27Z

Should we update the breadcrumb (to add the query path) in the same PR?

Signed-off-by: gfanton <[email protected]>

gfanton · 2024-12-20T14:31:53Z

@alexiscolin I will do this in another PR ;)

gno.land/pkg/gnoweb/url.go

thehowl · 2024-12-20T21:23:40Z

gno.land/pkg/gnoweb/url.go

-		default:
-			return nil, fmt.Errorf("%w: %s", ErrURLMalformedPath, u.Path)
-		}
+	if !rePkgOrRealmPath.MatchString(upath) {


What if we used gnolang.IsPurePackagePath / gnolang.IsRealmPath instead?

To check the string path directly? Or do you mean removing the check and using IsPurePackagePath and IsRealmPath? I would like to have IsPurePackagePath and IsRealmPath; I think it's a better idea than Kind(), but I'm not sure why you pointed out this specific line.

I just had beef with this regex and this seemed a good place where to make a comment :D

But yeah, I think we can also substitute this regex to determine if it's a valid path?

I don't think we should forbid a URL that is neither pure nor a realm. I prefer this package to perform minimal checks; it should be the caller's choice to accept the URL based on the parsed information. That said, we can still establish a minimal base to work on without assuming any urls.

What do you think about:

Removing the concept of "kind" within the structure while still providing helpers to check if the method is pure or a realm, as you mentioned.

Adding a new File field, trimming any file from the path when parsing and adding it to the structure.

Refining the regex to define what a path can be, based on what we have in gnovm/pkg/gnolang/helpers.go, with the only base constraint that it must start with a slash and at least one character. This will makes the check and the assumption of the path structure simple and easy to manage.

var rePkgOrRealmPath = regexp.MustCompile(`^/[a-z][a-z0-9_./]*$`)

Only pure and realm exist, but gnoweb will likely extend this with /u, for example. So it makes sense to me to make GnoURL a bit more flexible in what it can parse. This way, the caller can decide whether to use it as a direct call to the node or to redirect the user to something else.

@thehowl ~~suggestion preview: gfanton@d0eb4b0~~

I've directly pushed the commit 8f93987 with suggested changes. If you think it's appropriate, feel free to merge it. Otherwise, please don't hesitate to give me feedback so I can make the necessary updates.

edit: i've made another commit (62b005a) to handle special file, such as LICSENSE

gno.land/pkg/gnoweb/url.go

Co-authored-by: Morgan <[email protected]>

- Removing the concept of "kind" within the structure while still providing helpers to check if the method is pure or a realm: - Adding a new File field, trimming any file from the path when parsing and adding it to the structure. - Refining the regex to define what a path can be, based on what we have in `gnovm/pkg/gnolang/helpers.go` "var rePkgOrRealmPath = regexp.MustCompile(`^/[a-z][a-z0-9_/]*$`)" Signed-off-by: gfanton <[email protected]>

Signed-off-by: gfanton <[email protected]>

zivkovicmilos

Looks good 💯

Feel free to merge 🚀

zivkovicmilos · 2025-01-06T12:09:14Z

gno.land/pkg/gnoweb/app_test.go

@@ -125,7 +126,7 @@ func TestAnalytics(t *testing.T) {
 				request := httptest.NewRequest(http.MethodGet, route, nil)
 				response := httptest.NewRecorder()
 				router.ServeHTTP(response, request)
-				fmt.Println("HELLO:", response.Body.String())


zivkovicmilos · 2025-01-06T12:13:22Z

gno.land/pkg/gnoweb/url.go

+		// but this may require changes in some realms.
+		args := gnoURL.Args
+		if !encodeFlags.Has(EncodeNoEscape) {
+			args = escapeDollarSign(url.PathEscape(args))


Nitpick: you only call this func once

zivkovicmilos · 2025-01-06T12:15:43Z

gno.land/pkg/gnoweb/url.go

 }

-func (url GnoURL) EncodePath() string {
+// Encode encodes the URL components based on the provided flags.
+func (gnoURL GnoURL) Encode(encodeFlags EncodeFlag) string {


What do you think about adding a small section on the gnoweb docs for these URL flags?

gfanton added 5 commits December 18, 2024 13:25

fix: simplify url system

f82ec43

Signed-off-by: gfanton <[email protected]>

wip: more tests

e28401a

Signed-off-by: gfanton <[email protected]>

fix: improve url

4c6535a

Signed-off-by: gfanton <[email protected]>

feat: improve url tests

5d855d7

Signed-off-by: gfanton <[email protected]>

Merge branch 'master' into fix/fgnoweb/simplify-url

039b9ed

github-actions bot assigned gfanton Dec 18, 2024

github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Dec 18, 2024

gfanton changed the title ~~fix(gnoweb): simplify url system~~ fix(gnoweb): simplify url parsing system Dec 18, 2024

fix: simplify more and add comments

9f7b111

Signed-off-by: gfanton <[email protected]>

gfanton added 2 commits December 18, 2024 17:14

chore: lint

d44ea6b

Signed-off-by: gfanton <[email protected]>

Merge branch 'master' into fix/fgnoweb/simplify-url

d809304

Merge branch 'master' into fix/fgnoweb/simplify-url

7db64ce

gfanton mentioned this pull request Dec 19, 2024

chore(gnoweb): cleanup iteration on gnoweb #3379

Open

feat: add bitwise flags to encode url

6a07ed2

Signed-off-by: gfanton <[email protected]>

Kouteki requested review from thehowl, zivkovicmilos and leohhhn December 20, 2024 12:03

Kouteki added the in focus Core team is prioritizing this work label Dec 20, 2024

Kouteki requested a review from alexiscolin December 20, 2024 12:04

gfanton added 4 commits December 20, 2024 13:37

fix: improve url encoding + tests

ed3709c

Signed-off-by: gfanton <[email protected]>

feat(url): add IsValid method

3a5a71c

Signed-off-by: gfanton <[email protected]>

chore: lint

6ffc310

Signed-off-by: gfanton <[email protected]>

Merge branch 'master' into fix/fgnoweb/simplify-url

0c41f48

Merge branch 'master' into fix/fgnoweb/simplify-url

c88c186

thehowl reviewed Dec 20, 2024

View reviewed changes

Update gno.land/pkg/gnoweb/url.go - typo

bff4a0b

Co-authored-by: Morgan <[email protected]>

gfanton added 2 commits December 23, 2024 16:01

Merge branch 'master' into fix/fgnoweb/simplify-url

90318c7

gfanton force-pushed the fix/fgnoweb/simplify-url branch from 8d51e3b to 8f93987 Compare December 26, 2024 15:31

Merge branch 'master' into fix/fgnoweb/simplify-url

c51ec01

gfanton requested a review from thehowl January 6, 2025 10:16

fix: check for upcase string as file

62b005a

Signed-off-by: gfanton <[email protected]>

gfanton force-pushed the fix/fgnoweb/simplify-url branch from c6e36df to 62b005a Compare January 6, 2025 10:36

zivkovicmilos approved these changes Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gnoweb): simplify url parsing system #3366

fix(gnoweb): simplify url parsing system #3366

gfanton commented Dec 18, 2024 •

edited

Loading

Gno2D2 commented Dec 18, 2024 •

edited

Loading

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Can be checked by

codecov bot commented Dec 18, 2024 •

edited

Loading

alexiscolin commented Dec 19, 2024

gfanton commented Dec 20, 2024

thehowl Dec 20, 2024

gfanton Dec 20, 2024

thehowl Dec 20, 2024

thehowl Dec 20, 2024

gfanton Dec 22, 2024 •

edited

Loading

gfanton Dec 23, 2024 •

edited

Loading

zivkovicmilos left a comment

zivkovicmilos Jan 6, 2025

zivkovicmilos Jan 6, 2025

zivkovicmilos Jan 6, 2025

fix(gnoweb): simplify url parsing system #3366

Are you sure you want to change the base?

fix(gnoweb): simplify url parsing system #3366

Conversation

gfanton commented Dec 18, 2024 • edited Loading

Gno2D2 commented Dec 18, 2024 • edited Loading

🛠 PR Checks Summary

Manual Checks (for Reviewers):

✅ Automated Checks (for Contributors):

☑️ Contributor Actions:

☑️ Reviewer Actions:

📚 Resources:

If

Then

If

Can be checked by

codecov bot commented Dec 18, 2024 • edited Loading

Codecov Report

alexiscolin commented Dec 19, 2024

gfanton commented Dec 20, 2024

thehowl Dec 20, 2024

Choose a reason for hiding this comment

gfanton Dec 20, 2024

Choose a reason for hiding this comment

thehowl Dec 20, 2024

Choose a reason for hiding this comment

thehowl Dec 20, 2024

Choose a reason for hiding this comment

gfanton Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

gfanton Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

zivkovicmilos left a comment

Choose a reason for hiding this comment

zivkovicmilos Jan 6, 2025

Choose a reason for hiding this comment

zivkovicmilos Jan 6, 2025

Choose a reason for hiding this comment

zivkovicmilos Jan 6, 2025

Choose a reason for hiding this comment

gfanton commented Dec 18, 2024 •

edited

Loading

Gno2D2 commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading

gfanton Dec 22, 2024 •

edited

Loading

gfanton Dec 23, 2024 •

edited

Loading