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

Reserve 'c' for the client's receiver name #2144

Merged
merged 9 commits into from Jul 1, 2019

Conversation

@ikawaha
Copy link
Contributor

commented Jun 6, 2019

Fix #2138.

Design

var _ = Service("calc", func() {
	Description("The calc service performs operations on numbers.")

	Method("add", func() {
		Payload(func() {
			Field(1, "a", Int, "Left operand")
			Field(2, "c", Int, "Right operand")
		})

		Result(Int)

		HTTP(func() {
			GET("/add/{a}/{c}")  // ← ★
		})
	})
})

Generated code

// BuildAddRequest instantiates a HTTP request object with method and path set
// to call the "calc" service "add" endpoint
func (c *Client) BuildAddRequest(ctx context.Context, v interface{}) (*http.Request, error) {
	var (
		a  int
		c2 int // ← ★
	)
	{
		p, ok := v.(*calc.AddPayload)
		if !ok {
			return nil, goahttp.ErrInvalidType("calc", "add", "*calc.AddPayload", v)
		}
		if p.A != nil {
			a = *p.A
		}
		if p.C != nil {
			c2 = *p.C
		}
	}
	u := &url.URL{Scheme: c.scheme, Host: c.host, Path: AddCalcPath(a, c2)}
	req, err := http.NewRequest("GET", u.String(), nil)
	if err != nil {
		return nil, goahttp.ErrInvalidURL("calc", "add", u.String(), err)
	}
	if ctx != nil {
		req = req.WithContext(ctx)
	}

	return req, nil
}
ikawaha added 2 commits Jun 6, 2019

@ikawaha ikawaha changed the title Reserve 'c' for the client's receiver name [WIP]Reserve 'c' for the client's receiver name Jun 6, 2019

@@ -2184,7 +2191,7 @@ func extractPathParams(a *expr.MappedAttributeExpr, service *expr.AttributeExpr,
StringSlice: arr != nil && arr.ElemType.Type.Kind() == expr.StringKind,
Map: false,
MapStringSlice: false,
Validate: codegen.RecursiveValidationCode(c, ctx, true, varn),
Validate: codegen.RecursiveValidationCode(c, ctx, true, varn, name),

This comment has been minimized.

Copy link
@ikawaha

ikawaha Jun 6, 2019

Author Contributor

Setting original path param name, to avoid setting escaped name to the validation target.

before:

err = goa.MergeErrors(err, goa.ValidatePattern("c2", c2, "patternc")) 

after:

err = goa.MergeErrors(err, goa.ValidatePattern("c", c2, "patternc"))

There are several places where it is better to use the original name.
If this PR is adopted, correct them as well.

$ git grep codegen.RecursiveValidationCode|grep varn
grpc/codegen/service_data.go:			Validate:     codegen.RecursiveValidationCode(c, ctx, required, varn),
http/codegen/service_data.go:					Validate:       codegen.RecursiveValidationCode(pAtt, httpsvrctx, required, varn),
http/codegen/service_data.go:			Validate:     codegen.RecursiveValidationCode(c, ctx, required, varn),
http/codegen/service_data.go:			Validate:      codegen.RecursiveValidationCode(hattr, svcCtx, required, varn),

@ikawaha ikawaha changed the title [WIP]Reserve 'c' for the client's receiver name Reserve 'c' for the client's receiver name Jun 6, 2019

@nitinmohan87
Copy link
Contributor

left a comment

Thanks for the PR!

Since you introduced a new parameter to RecursiveValidationCode this becomes an interface-breaking change. I am thinking if we can try to avoid making interface-breaking change for this particular bug.

Only the BuildXXXRequest gets this problem because of the client handle variable c. So we can simply modify this particular section to not conflict with the client handle variable, instead of doing it all over the generated code. I think it is just about checking if any of the routes[0].PathInit.ClientArgs has a name c and producing a new name for it. Also, we may need to modify this template to use .Args instead of .PathInit.ClientArgs where necessary. I think this will eliminate any interface-breaking changes. Please let me know if that makes sense!

ikawaha added 3 commits Jun 14, 2019
Rollback
Revert following commits:
05bf59f Update
d35a12e Use original path param name
7a0c5ce Add optional context parameter to RecursiveValidationCode function
a8b9a22 go fmt
f498a16 Reserve 'c' for the client's receiver name

This reverts commit f498a16.
@ikawaha

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

Thank you for your review!
I tried to fix it in the way you suggested. I hope it makes sense.
There may be a better way. If so please close this pr.
Thank you.


Example desgin

package design

import (
   . "goa.design/goa/v3/dsl"
)

var _ = API("calc", func() {
   Title("Calculator Service")
   Description("Service for adding numbers, a Goa teaser")
   Server("calc", func() {
      Host("localhost", func() {
         URI("http://localhost:8000")
      })
   })
})

var _ = Service("calc", func() {
   Description("The calc service performs operations on numbers.")

   Method("add", func() {
      Payload(func() {
         Field(1, "a", Int)
         Field(5, "b", Int)
         Field(2, "c", Int)
         Field(3, "c1", Int)
         Field(4, "c_", Int)
      })
      Result(Int)
      HTTP(func() {
         GET("/add/{a}/{b}/{c}/{c1}/{c_}")  // ← ★
      })
   })
})

Generated code

// BuildAddRequest instantiates a HTTP request object with method and path set
// to call the "calc" service "add" endpoint
func (c *Client) BuildAddRequest(ctx context.Context, v interface{}) (*http.Request, error) {
   var (
      a   int
      b   int
      c_  int
      c1  int
      c_2 int
   )
   {
      p, ok := v.(*calc.AddPayload)
      if !ok {
         return nil, goahttp.ErrInvalidType("calc", "add", "*calc.AddPayload", v)
      }
      if p.A != nil {
         a = *p.A
      }
      if p.B != nil {
         b = *p.B
      }
      if p.C != nil {
         c_ = *p.C
      }
      if p.C1 != nil {
         c1 = *p.C1
      }
      if p.C != nil {
         c_2 = *p.C
      }
   }
   u := &url.URL{Scheme: c.scheme, Host: c.host, Path: AddCalcPath(a, b, c_, c1, c_2)}
   req, err := http.NewRequest("GET", u.String(), nil)
   if err != nil {
      return nil, goahttp.ErrInvalidURL("calc", "add", u.String(), err)
   }
   if ctx != nil {
      req = req.WithContext(ctx)
   }

   return req, nil
}
for _, ca := range routes[0].PathInit.ClientArgs {
if ca.FieldName != "" {
ca.Name = s.Unique(ca.Name, "_")

This comment has been minimized.

Copy link
@nitinmohan87

nitinmohan87 Jun 14, 2019

Contributor

Instead of using Unique you can use Name which will automatically suffix a counter variable to the name. You don't have to suffix with _

ca.Name = s.Name(ca.Name)
@nitinmohan87

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

Looks great! One minor comment and then this is good to go.

@raphael

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

Thank you!

@raphael raphael merged commit 5b5be3f into goadesign:v3 Jul 1, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@raphael

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

@ikawaha do you think you could backport this fix to v2? (simply cherry-pick the merge commit into a branch created off of v2). Thank you!

ikawaha added a commit to ikawaha/goa that referenced this pull request Jul 1, 2019
Reserve 'c' for the client's receiver name (goadesign#2144)
* Reserve 'c' for the client's receiver name

* go fmt

* Add optional context parameter to RecursiveValidationCode function

* Use original path param name

* Update

* Rollback

Revert following commits:
05bf59f Update
d35a12e Use original path param name
7a0c5ce Add optional context parameter to RecursiveValidationCode function
a8b9a22 go fmt
f498a16 Reserve 'c' for the client's receiver name

This reverts commit f498a16.

* Fix variable names in BuildXXXRequest

* Use .Args instead of .PathInit.ClientArgs

* Remove the suffix argument
raphael added a commit that referenced this pull request Jul 1, 2019
Reserve 'c' for the client's receiver name (#2144) (#2170)
* Reserve 'c' for the client's receiver name

* go fmt

* Add optional context parameter to RecursiveValidationCode function

* Use original path param name

* Update

* Rollback

Revert following commits:
05bf59f Update
d35a12e Use original path param name
7a0c5ce Add optional context parameter to RecursiveValidationCode function
a8b9a22 go fmt
f498a16 Reserve 'c' for the client's receiver name

This reverts commit f498a16.

* Fix variable names in BuildXXXRequest

* Use .Args instead of .PathInit.ClientArgs

* Remove the suffix argument
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.