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

Support Parameter Objects #71

Merged
merged 3 commits into from Jun 1, 2017
Merged

Support Parameter Objects #71

merged 3 commits into from Jun 1, 2017

Conversation

abhinav
Copy link
Collaborator

@abhinav abhinav commented May 26, 2017

This adds support for parameter objects to Dig. Parameter objects are arguments
to a constructor which contain zero or more fields that are the actual
dependencies for the constructor. Users opt-in for a struct to be interpreted
as a parameter object by embedding the dig.Param type into it.

type Config struct {
    dig.Param

    In io.Reader
    Out io.Writer
}

func NewTransport(c Config) (*Transport, error) {
    // ..
}

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 90.213% when pulling 80fdf72 on parameter-objects into 13204cd on ajs-fix-behavior-tests.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 90.213% when pulling 80fdf72 on parameter-objects into 13204cd on ajs-fix-behavior-tests.

@glibsm glibsm mentioned this pull request May 30, 2017
@abhinav abhinav changed the base branch from ajs-fix-behavior-tests to ajs-add-behavior-tests May 30, 2017 20:47
@abhinav abhinav changed the base branch from ajs-add-behavior-tests to ajs-fix-behavior-tests May 30, 2017 20:47
@coveralls
Copy link

coveralls commented May 30, 2017

Coverage Status

Coverage increased (+0.9%) to 90.129% when pulling 1a0b326 on parameter-objects into cc127cb on ajs-fix-behavior-tests.

dig.go Outdated

t := ctype.In(i)
if t.Implements(_parameterObjectType) {
arg, err = getParameterObject(c, t)
Copy link

Choose a reason for hiding this comment

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

Why getParameterObject is a function, not a method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No reason. Changed.

dig.go Outdated
continue
}

// THe user added a parameter object as a dependency. We don't recurse
Copy link

Choose a reason for hiding this comment

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

nit: The

dig.go Outdated

// THe user added a parameter object as a dependency. We don't recurse
// /yet/ so let's try to give an informative error message.
if f.Type.Implements(_parameterObjectType) {
Copy link

Choose a reason for hiding this comment

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

I've tried private implementations and it looks very confusing:
foo/foo.go:

package foo

type A struct {}

func (A) private() {
	println("foo")
}

main.go

package main

import (
	"fmt"
	"tmp/foo"
	"reflect"
)

type B struct{foo.A}
type private interface {
	private()
}

func main() {
	b := reflect.ValueOf(B{})
	v := (*private)(nil)
	fmt.Println(reflect.TypeOf(v).Elem())
	fmt.Println(b.Type().Implements(reflect.TypeOf(v).Elem()))
}

the output is:

main.private
true

May be I am doing something wrong, but it seems there could be conflicts if a type has parameterObject() method..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interfaces with unexported methods may only be implemented by types defined in
that package. A type that implements a private interface may be embedded
inside another type (which may reside outside the package containing the
private interface). Because methods of embedded types are "inherited", the
type that embeds the other type now implements the private interface too.

That's exactly the behavior we want: Only types that embed dig.Param
implement parameterObject. Demonstration:

// foo/foo.go
package foo

type A interface { private() }

func Matches(x interface{}) bool {
	_, ok := x.(A)
	return ok
}

// External types embedding this will match.
type EmbedMe struct{}

func (EmbedMe) private() {}
// main.go
package main

import (
	"fmt"
	"tmp/foo"
)

type bad struct{}

func (bad) private() {}

type good struct{ foo.EmbedMe }

func main() {
	fmt.Println("bad:", foo.Matches(bad{}))
	fmt.Println("good:", foo.Matches(good{}))
	// Output:
	// bad: false
	// good: true
}

In your example, the other package doesn't define the interface, only the
method. And you embed it in a type inside the package where you define the
interface, so it somehow matches.

This will not work for types that simply implement parameterObject().

Copy link

Choose a reason for hiding this comment

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

I got the trick you are doing, what I am arguing is that Implements(which you are using here) doesn't follow the same behavior as type assertion. If you replace Matches function with it:

func Matches(x interface{}) bool {
	return reflect.TypeOf(x).Implements(reflect.TypeOf((*A)(nil)).Elem())
}

Output is going to be:

bad: true
good: true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, wow. That's a bug in reflect. Good catch!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@alsamylkin Created golang/go#20541. Thanks!

"could not get field %v (type %v) of %v: %v", f.Name, f.Type, t, err)
}

dest.Field(i).Set(v)
Copy link

Choose a reason for hiding this comment

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

Do we need to check if the field is settable? (In case dig will support non-pointer types)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The field will always be settable because the struct that contains it is addressable (because we created it with reflect.New).

@@ -113,6 +115,9 @@ func (c *Container) provideInstance(val interface{}) error {
if vtype == _errType {
return errors.New("can't provide errors")
}
if vtype.Implements(_parameterObjectType) {
return errors.New("can't provide parameter objects")
Copy link
Contributor

Choose a reason for hiding this comment

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

parameter term is super generic. Can this error be more explicit? can't provide parameterized object

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've been using the term "parameter object" for types that embed dig.Param in all the documentation and errors so far. That's the standard name for objects which represent all the parameter of a function, created solely to avoid having too many arguments on the function. See http://wiki.c2.com/?ParameterObject, https://refactoring.com/catalog/introduceParameterObject.html.

We can call this something else, maybe give it a dig-specific name, but "parameterized object" has a whole different meaning.


t := ctype.In(i)
if t.Implements(_parameterObjectType) {
arg, err = c.getParameterObject(t)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: getting parameterObject can be part of c.get(t). Then you don't need to declare variables and assign. implementation wise we just want an object back from container.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call. Updating the other PR with this.

var deps []reflect.Type
for i := 0; i < t.NumField(); i++ {
f := t.Field(i)
if f.PkgPath != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: refactor? pkgPath and Anonymous is been checked in multiple places.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately that's unavoidable. These are two different entry points. This check is necessary to make sure we don't muck with private fields.

@coveralls
Copy link

coveralls commented Jun 1, 2017

Coverage Status

Coverage increased (+0.2%) to 91.845% when pulling 946fc3d on parameter-objects into c2597a2 on ajs-fix-behavior-tests.

@abhinav abhinav changed the base branch from ajs-fix-behavior-tests to dev June 1, 2017 19:16
This adds support for parameter objects to dig. Structs opt-in to be
treated as parameter objects by embedding `dig.Param`.

For the sake of simplicity from the users' point-of-view, we don't
recurse into parameter objects inside other parameter objects. This can
be changed in the future if there's demand for it.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 91.845% when pulling 5720cb9 on parameter-objects into da048de on dev.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 91.845% when pulling 5720cb9 on parameter-objects into da048de on dev.

@abhinav abhinav merged commit a0a83b4 into dev Jun 1, 2017
@abhinav abhinav deleted the parameter-objects branch June 1, 2017 19:18
abhinav pushed a commit that referenced this pull request Jun 1, 2017
The refactor and Params made it quite trivial to add support and e2e
test for optional tags.

Reading the test explains the expected behavior quite well.

Stacked on #70, #72, #71
hbdf pushed a commit to hbdf/dig that referenced this pull request Jul 14, 2022
* Disable overcommit signatures

Fixes uber-go#71

* Update contribution guide
hbdf pushed a commit to hbdf/dig that referenced this pull request Jul 14, 2022
…andling (uber-go#71)

* Added filter chain for HTTP module and filters for tracer and error

* Added http test to check that tracer has been set in the request context

* License and doc updates

* Undo doc changes

* Added filters test

* Remove redundant todo comments

* Inject the span context for the request in the tracing filter

* Use global logger and rename to panicFilter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants