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

Using Param() to map path parameters to attributes results in a runtime error #3462

Open
Makirchn opened this issue Jan 23, 2024 · 5 comments · Fixed by #3476
Open

Using Param() to map path parameters to attributes results in a runtime error #3462

Makirchn opened this issue Jan 23, 2024 · 5 comments · Fixed by #3476

Comments

@Makirchn
Copy link

Makirchn commented Jan 23, 2024

Hi, I'm new to goa and I want to map a path param e.g. product_id to a payload attribute id.

My design:

Service:

var _ = Service("products", func() {
	Description("The product service provides an API for managing products.")
	HTTP(func() {
		Path("/products")
	})

	// update product by id
	Method("update by id", func() {
		Payload(UpdateProductPayload)
		Result(UpdateProductPayload, func() {
			View("default")
		})
		Error("no_criteria", String, "Missing criteria")
		Error("no_match", String, "No product found with specified id")
		HTTP(func() {
			POST("/{product_id}")
			Param("product_id:id")
			Response(StatusOK)
			Response("no_criteria", StatusBadRequest)
			Response("no_match", StatusNotFound)
		})
	})
})

Payload Model:

var UpdateProductPayload = Type("UpdateProductPayload", func() {
	Attribute("id", String, "Product ID", func() {
		Example("83d83ec2-d2ca-49ff-bbea-b92b5c3be202")
	})
	Attribute("name", String, "Product Name", func() {
		Example("Apple")
	})
})

Generating the code results in:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x60 pc=0x1023a3028]
@raphael
Copy link
Member

raphael commented Feb 18, 2024

Just a note that there is indeed a bug in that Goa should never panic. That being said the correct DSL here would be:

var _ = Service("products", func() {
	Description("The product service provides an API for managing products.")
	HTTP(func() {
		Path("/products")
	})

	// update product by id
	Method("update by id", func() {
		Payload(UpdateProductPayload)
		Result(UpdateProductPayload, func() {
			View("default")
		})
		Error("no_criteria", String, "Missing criteria")
		Error("no_match", String, "No product found with specified id")
		HTTP(func() {
			POST("/{id}")
			Response(StatusOK)
			Response("no_criteria", StatusBadRequest)
			Response("no_match", StatusNotFound)
		})
	})
})

Param defines query string parameters so in the original DSL the id field gets mapped twice - once in the HTTP route and once as a query string parameter. So Param should be removed and instead the mapping done within the route definition (POST("/{id}")).

@Makirchn
Copy link
Author

So as I understand your answer, it is not possible to map a path param element name to an attribute name like it is possible for query, header or body elements?

@raphael
Copy link
Member

raphael commented Feb 19, 2024

Yes you are correct, the name of the path param needs to match the name of the attribute. The idea is that the name of the path params should not affect how clients build requests. I am curious to understand the use case for wanting to use a different name for the path param than the name of the attribute?

@Makirchn
Copy link
Author

I saw the http element mapping of header, body and query elements on https://goa.design/design/http_mapping/ and simply tried to use it with path parameters. My intention was to use a speaking route like /users/{user_id}/products/{product_id} instead of using /users/{id}/products/{product_id} while having a consistent schema for my structs by having them all have an id parameter regardless of whether it is a user, product or is something else.

@raphael raphael reopened this Feb 21, 2024
@raphael
Copy link
Member

raphael commented Feb 21, 2024

That's fair, it does set the expectation. I guess the one place where that might matter is in the generated OpenAPI specification. I'm reopening the issue so we can add this feature.

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 a pull request may close this issue.

2 participants