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

Adding trailing / in path server.go #2557

Closed
cocktailer opened this issue May 12, 2020 · 11 comments
Closed

Adding trailing / in path server.go #2557

cocktailer opened this issue May 12, 2020 · 11 comments
Labels

Comments

@cocktailer
Copy link

cocktailer commented May 12, 2020

image
This is breaking my existing app. Why do we introducing "/" in the end

@nitinmohan87 @raphael

@cocktailer cocktailer changed the title Adding trail / in path server.go Adding trailing / in path server.go May 12, 2020
@raphael
Copy link
Member

raphael commented May 12, 2020

@cocktailer is the "/" added or is it present in the design? #2530 fixed the fact that the slash wasn't generated even though it was set in the design. However it shouldn't be added if it wasn't. If it is added and not present in the design can you please post a small repro (a small but complete design we can use to generate the code)? Thank you!

@saniales
Copy link
Member

As far as I know this is expected behavior if you set the path WITHOUT the / in the design

@cocktailer
Copy link
Author

@saniales trailing / in /storage/ is not needed. Why are we adding the trailing / in path even though not added in design?

@raphael
Copy link
Member

raphael commented May 14, 2020

I see I missed the case where the path is just a slash. Seems we should handle this case specially and not add the trailing slash when the path is just "/". @ikawaha do you agree?. You could still have a trailing slash by adding it to the base path:

var _ = Service("base_path_no_trailing", func() {
    HTTP(func() { Path("foo") })
    Method("slash_with_base_path_no_trailing", func() {
        HTTP(func() { GET("/") })
    })
    Method("trailing_with_base_path_no_trailing", func() {
        HTTP(func() { POST("/bar/") })
    })
})
var _ = Service("base_path_with_trailing", func() {
    HTTP(func() { Path("foo/") })
    Method("slash_with_base_path_with_trailing", func() {
        HTTP(func() { GET("/") })
    })
})
var _ = Service("no_base_path", func() {
    Method("slash_no_base_path", func() {
        HTTP(func() { GET("/") })
    })
    Method("trailing_no_base_path", func() {
        HTTP(func() { POST("/foo/") })
    })
})

Would result in the paths:

slash_with_base_path_no_trailing: /foo --> ** the case that needs to change **
trailing_with_base_path_no_trailing: /foo/bar/
slash_with_base_path_with_trailing: /foo/
slash_no_base_path: /
trailing_no_base_path: /foo/

(in every case if the method defines a path that does not have a trailing slash then the resulting route would not have one either)

@ikawaha
Copy link
Contributor

ikawaha commented May 14, 2020

I agree. It seems to be a bug.

slash_with_base_path_no_trailing: /foo --> ** the case that needs to change **

@saniales saniales added the bug label May 15, 2020
@ikawaha
Copy link
Contributor

ikawaha commented May 15, 2020

I may be mistaken in saying this is a bug.

When a base path is specified and a '/' is set to the endpoint, it seems better not to add a trailing slash, but you can't explicitly add a trailing slash to the base path.

I was reviewing the Goa v1 specs, and when using the base path as-is for an endpoint, I was specifying an empty string for the endpoint. This may seem like a strange designation, but this may be a more reasonable way of doing things.

If you decide not to add trailing slash when '/' is set, you need a special symbol (e.g., '//') to specify that trailing slash should be given.

@kumarsiva07
Copy link

kumarsiva07 commented May 15, 2020

var _ = Service("base_path_no_trailing", func() {
    HTTP(func() { Path("foo") })
    Method("slash_with_base_path_no_trailing", func() {
        HTTP(func() { GET("/") })
    })
})

I am expecting a path like "/foo" for the above design. But I am getting /foo/.
So I think this is a bug. @ikawaha

@ikawaha
Copy link
Contributor

ikawaha commented May 15, 2020

var _ = Service("base_path_no_trailing", func() {
    HTTP(func() { Path("foo") })
    Method("slash_with_base_path_no_trailing", func() {
        HTTP(func() { GET("") }) // ← 
    })
})

This design generate /foo. But, it looks odd to specify an empty string as an endpoint. This may be a spec issue and not a bug.

@kumarsiva07
Copy link

kumarsiva07 commented May 15, 2020

Do you think https://github.com/goadesign/examples/blob/v2/cellar/design/sommelier.go design also an issue? So far we followed these kinds of design. @ikawaha @raphael

@raphael
Copy link
Member

raphael commented May 23, 2020

I think this issue can be closed. To summarize: if a relative path ends with / then it is preserved except in the case where the relative path is just /. In this case the slash is not added to the base path. The slash can be added by using ./ instead of /.

@raphael raphael closed this as completed May 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants