Skip to content
This repository has been archived by the owner on Oct 14, 2021. It is now read-only.

feat: Add support for http://example.com #17

Merged
merged 4 commits into from
Sep 27, 2021
Merged

feat: Add support for http://example.com #17

merged 4 commits into from
Sep 27, 2021

Conversation

Joey-1445601153
Copy link
Contributor

fix:Add support for :// #15

@Xuanwo Xuanwo changed the title fix:Add support for :// #15 fix:Add support for :// Sep 26, 2021
@Xuanwo Xuanwo changed the title fix:Add support for :// fix: Add support for :// Sep 26, 2021
"tcp://127.0.0.1:xxx",
Endpoint{},
ErrInvalidValue,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add test cases like:

  • file:///tmp/test

@@ -33,6 +33,12 @@ func TestParse(t *testing.T) {
Endpoint{ProtocolHTTP, hostPort{"example.com", 80}},
nil,
},
{
"normal http with multi /",
"http://////example.com:80",
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be invalid?

endpoint.go Outdated
@@ -44,6 +44,10 @@ func Parse(cfg string) (p Endpoint, err error) {
// Handle file paths that contains ":" (often happens on windows platform)
//
// See issue: https://github.com/beyondstorage/go-endpoint/issues/3
s[1] = strings.TrimLeft(s[1], "/")
Copy link
Contributor

Choose a reason for hiding this comment

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

Compared to handle them in different protocol cases, how about handling all of them? Maybe we can operate on s[1] directly?

@Joey-1445601153
Copy link
Contributor Author

@Xuanwo
for
"normal http with multi /",
"http://////example.com:80",

I tried using address like http:////74.125.128.160:80 to visit google,it's valid.
I also create a simple local server. When I used address http:////127.0.0.1:8080/ to visit it, it was valid too.
So maybe it's valid for all?

@Xuanwo
Copy link
Contributor

Xuanwo commented Sep 27, 2021

http:////74.125.128.160:80

They may be valid for the browser, but they are not valid according to RFC3986: https://datatracker.ietf.org/doc/html/rfc3986#section-3

@Joey-1445601153
Copy link
Contributor Author

@Xuanwo
For the problem in endpoint.go, handling all of them together may makes code chaos.
How about use a method like parseFile(imitate parseHostPort) to encapsulate these lines?

@Xuanwo
Copy link
Contributor

Xuanwo commented Sep 27, 2021

may makes code chaos

Please statement it more clear. Which problem could happen?

@Joey-1445601153
Copy link
Contributor Author

may makes code chaos

Please statement it more clear. Which problem could happen?

If handling all of them together, we have to consider several situations in one method.The logic may be complex.One process may be entangled with another. It's hard to read and maintain.

@Xuanwo
Copy link
Contributor

Xuanwo commented Sep 27, 2021

we have to consider several situations in one method

Let's discuss them here now.

From my point of view, we only need to check like the following:

if strings.HasPrefix(s[1], "//") {
    s[1] = s[1][2:]
}

Please correct me if there are other situations I missed.

@Joey-1445601153
Copy link
Contributor Author

Joey-1445601153 commented Sep 27, 2021

http:////74.125.128.160:80

They may be valid for the browser, but they are not valid according to RFC3986: https://datatracker.ietf.org/doc/html/rfc3986#section-3

we have to consider several situations in one method

Let's discuss them here now.

From my point of view, we only need to check like the following:

if strings.HasPrefix(s[1], "//") {
    s[1] = s[1][2:]
}

Please correct me if there are other situations I missed.
If we just cut two headmost '/' of s[1], it's OK.
when whole string is http:///example.com, use s[1] = s[1][2:] will make the result of ep.HTTP() is http:///example.com again, i think the result is not respecting the // notation. According to RFC3986, it's invalid. It's valid for browser, wo also have to consider about the different and how to process. To be honest, i 'm comfused about it.

Uniform Resource Identifier (URI): Generic Syntax (RFC )

@Xuanwo
Copy link
Contributor

Xuanwo commented Sep 27, 2021

http:///example.com

We will return errors for invalid input. For example, we can return an error if parseHostPort got a string start with a /.

@Joey-1445601153
Copy link
Contributor Author

http:///example.com

We will return errors for invalid input. For example, we can return an error if parseHostPort got a string start with a /.

Oh, so accutualy we can process s[1] before switch s[0].Then we don't have to change the method parseHostPort.
For http&https, we don‘t support input like http:///example.com.We just return an err for this kind of input.
And for file ,file:///tmp is valid, we encapsulate a method like parseFile to process the different between windows and linux.
Do i understand correctly?

@Xuanwo
Copy link
Contributor

Xuanwo commented Sep 27, 2021

Do i understand correctly?

100% correct.

Besides, we don't need a parseFile here, let the caller handle the difference.

@Xuanwo Xuanwo changed the title fix: Add support for :// feat: Add support for http://example.com Sep 27, 2021
@Xuanwo Xuanwo merged commit ea2a3dd into beyondstorage:master Sep 27, 2021
@Xuanwo
Copy link
Contributor

Xuanwo commented Sep 27, 2021

Much better! Thanks for your hard working!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants