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 bug:about the MVC package route binding #1364

Merged
merged 3 commits into from
Oct 5, 2019

Conversation

BenLampson
Copy link
Contributor

@BenLampson BenLampson commented Sep 25, 2019

I found there has some little bug in the controller_method_parser.go
the bug is: if someone uses the code like this:
func (cc *HelloWorld) GetInfoXYT()
We can't create the Url that he wants.
We lost the T, our URL is: info/x/y
Cause I found the comment in this scope,
Like this:// it doesn't count the last uppercase
And I think the author tries to append the lastest Code, but the last append is
words = append(words, s[start:end])
You know, If the string is GetInfoXYT the end index is the same as the start, so we lost the T.
I think this processing mode can not be accepted, so I try to fix this.

We'd love to see more contributions

Read how you can contribute to the project.

Please attach an issue link which your PR solves otherwise your work may be rejected.

I found there has some little bug in the controller_method_parser.go
the bug is : if someone use the code like this:
func (cc *HelloWorld) GetInfoXYT()
We can't create the Url that he want.
We lose the T, our's url is: info/x/y
Cause// it doesn't count the last uppercase and the last append is
words = append(words, s[start:end])
@CLAassistant
Copy link

CLAassistant commented Sep 25, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Owner

@kataras kataras left a comment

Choose a reason for hiding this comment

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

Hello @BenLampson,

Did you find out that this is the solution to your problem? And also could you please add a test case at:

iris/mvc/controller_test.go

Lines 385 to 412 in df88227

type testControllerRelPathFromFunc struct{}
func (c *testControllerRelPathFromFunc) BeginRequest(ctx context.Context) {}
func (c *testControllerRelPathFromFunc) EndRequest(ctx context.Context) {
ctx.Writef("%s:%s", ctx.Method(), ctx.Path())
}
func (c *testControllerRelPathFromFunc) Get() {}
func (c *testControllerRelPathFromFunc) GetBy(uint64) {}
func (c *testControllerRelPathFromFunc) GetUint8RatioBy(uint8) {}
func (c *testControllerRelPathFromFunc) GetInt64RatioBy(int64) {}
func (c *testControllerRelPathFromFunc) GetAnythingByWildcard(string) {}
func (c *testControllerRelPathFromFunc) GetLogin() {}
func (c *testControllerRelPathFromFunc) PostLogin() {}
func (c *testControllerRelPathFromFunc) GetAdminLogin() {}
func (c *testControllerRelPathFromFunc) PutSomethingIntoThis() {}
func (c *testControllerRelPathFromFunc) GetSomethingBy(bool) {}
func (c *testControllerRelPathFromFunc) GetSomethingByBy(string, int) {}
func (c *testControllerRelPathFromFunc) GetSomethingNewBy(string, int) {} // two input arguments, one By which is the latest word.
func (c *testControllerRelPathFromFunc) GetSomethingByElseThisBy(bool, int) {} // two input arguments
func TestControllerRelPathFromFunc(t *testing.T) {
in this PR so I can accept it?

Thanks!

@BenLampson
Copy link
Contributor Author

Yeah~ In my project we have a Url resource to get the item's location, so our URL is: GET:api/v1/xxx/item/{int}/location/x

And my project already accepted this solution.

Copy link
Owner

@kataras kataras left a comment

Choose a reason for hiding this comment

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

OK @BenLampson ,

I've pushed a commit to your PR as well, just to know whenever you add a test controller method you should fire the requests as well:

	e.GET("/location/x").Expect().Status(iris.StatusOK).
		Body().Equal("GET:/location/x")
	e.GET("/location/x/y").Expect().Status(iris.StatusOK).
		Body().Equal("GET:/location/x/y")
	e.GET("/location/z/42").Expect().Status(iris.StatusOK).
		Body().Equal("GET:/location/z/42")

@kataras kataras merged commit 2f844a6 into kataras:master Oct 5, 2019
@BenLampson
Copy link
Contributor Author

BenLampson commented Oct 9, 2019

Thx for your tips~ I learn the more things from your proposal~
I will add the more things in my other commit next time~

@kataras
Copy link
Owner

kataras commented Oct 16, 2019

You are welcome @BenLampson, I thank you for your contribution!

@kataras kataras added this to the v12.0.0 milestone Oct 26, 2019
github-actions bot pushed a commit to goproxies/github.com-kataras-iris that referenced this pull request Jul 27, 2020
Former-commit-id: 836fb947642976f609f97cd5aeb4226e1fb991b8
github-actions bot pushed a commit to goproxies/github.com-kataras-iris that referenced this pull request Jul 27, 2020
Former-commit-id: 36560cd42e38af01ebe48e05745d71bbde981e12
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.

3 participants