don't create new controller instance when all fields are set-ed by the end-dev - but we keep show those fields' values as Dependencies on the BeforeActivate in order to the future custom controllers authors to be able to check if something is added as dependecy or even manually set-ed before bind their own dependecies, otherwise they could override the manually set-ing

Former-commit-id: 72d3a0f1299781ee9a5e3e35e4a543354f8cd63d

mvc/README.md

@@ -1,4 +1,4 @@
-# MVC Engine's Internals
+# MVC Internals
 
 * `MakeHandler` - accepts a function which accepts any input and outputs any result, and any optional values that will be used as binders, if needed they will be converted in order to be faster at serve-time. Returns a `context/iris#Handler` and a non-nil error if passed function cannot be wrapped to a raw `context/iris#Handler`
     * Struct fields with `Struct Binding`

mvc/controller.go

@@ -68,15 +68,6 @@ func newControllerActivator(router router.Party, controller interface{}, d *di.D
 		fullName = getNameOf(typ)
 	)
 
-	// the following will make sure that if
-	// the controller's has set-ed pointer struct fields by the end-dev
-	// we will include them to the bindings.
-	// set bindings to the non-zero pointer fields' values that may be set-ed by
-	// the end-developer when declaring the controller,
-	// activate listeners needs them in order to know if something set-ed already or not,
-	// look `BindTypeExists`.
-	d.Values = append(di.LookupNonZeroFieldsValues(val), d.Values...)
-
 	c := &ControllerActivator{
 		// give access to the Router to the end-devs if they need it for some reason,
 		// i.e register done handlers.
@@ -97,6 +88,19 @@ func newControllerActivator(router router.Party, controller interface{}, d *di.D
 		Dependencies:    d,
 	}
 
+	filledFieldValues := di.LookupNonZeroFieldsValues(val)
+	c.Dependencies.AddValue(filledFieldValues...)
+
+	if len(filledFieldValues) == di.IndirectType(typ).NumField() {
+		// all fields are filled by the end-developer,
+		// the controller doesn't contain any other field, not any dynamic binding as well.
+		// Therefore we don't need to create a new controller each time.
+		// Set the c.injector now instead on the first `Handle` and set it to invalid state
+		// in order to `buildControllerHandler` ignore
+		// creating new controller value on each incoming request.
+		c.injector = &di.StructInjector{Valid: false}
+	}
+
 	return c
 }
 
@@ -109,6 +113,28 @@ func whatReservedMethods(typ reflect.Type) []string {
 	return methods
 }
 
+// IsRequestScoped returns new if each request has its own instance
+// of the controller and it contains dependencies that are not manually
+// filled by the struct initialization from the caller.
+func (c *ControllerActivator) IsRequestScoped() bool {
+	// if the c.injector == nil means that is not seted to invalidate state,
+	// so it contains more fields that are filled by the end-dev.
+	// This "strange" check happens because the `IsRequestScoped` may
+	// called before the controller activation complete its task (see Handle: if c.injector == nil).
+	if c.injector == nil { // is nil so it contains more fields, maybe request-scoped or dependencies.
+		return true
+	}
+	if c.injector.Valid {
+		// if injector is not nil:
+		// if it is !Valid then all fields are manually filled by the end-dev (see `newControllerActivator`).
+		// if it is Valid then it's filled on the first `Handle`
+		// and it has more than one valid dependency from the registered values.
+		return true
+	}
+	// it's not nil and it's !Valid.
+	return false
+}
+
 // checks if a method is already registered.
 func (c *ControllerActivator) isReservedMethod(name string) bool {
 	for _, s := range c.reservedMethods {
@@ -224,7 +250,6 @@ func (c *ControllerActivator) Handle(method, path, funcName string, middleware .
 		if c.injector.Valid {
 			golog.Debugf("MVC dependencies of '%s':\n%s", c.FullName, c.injector.String())
 		}
-
 	}
 
 	if funcInjector.Valid {
@@ -267,9 +292,9 @@ func buildControllerHandler(m reflect.Method, typ reflect.Type, initRef reflect.
 
 		if !hasStructInjector {
 			// if the controller doesn't have a struct injector
-			// and the controller's fields are empty
+			// and the controller's fields are empty or all set-ed by the end-dev
 			// then we don't need a new controller instance, we use the passed controller instance.
-			if elemTyp.NumField() == 0 {
+
 			if !hasFuncInjector {
 				return func(ctx context.Context) {
 					DispatchFuncResult(ctx, initRef.Method(m.Index).Call(emptyIn))
@@ -287,26 +312,6 @@ func buildControllerHandler(m reflect.Method, typ reflect.Type, initRef reflect.
 				DispatchFuncResult(ctx, m.Func.Call(in))
 			}
 		}
-			// it has fields, so it's request-scoped, even without struct injector
-			// it's safe to create a new controller on each request because the end-dev
-			// may use the controller's fields for request-scoping, so they should be
-			// zero on the next request.
-			if !hasFuncInjector {
-				return func(ctx context.Context) {
-					DispatchFuncResult(ctx, reflect.New(elemTyp).Method(m.Index).Call(emptyIn))
-				}
-			}
-			return func(ctx context.Context) {
-				in := make([]reflect.Value, n, n)
-				in[0] = reflect.New(elemTyp)
-				funcInjector.Inject(&in, reflect.ValueOf(ctx))
-				if ctx.IsStopped() {
-					return
-				}
-
-				DispatchFuncResult(ctx, m.Func.Call(in))
-			}
-		}
 
 		// it has struct injector for sure and maybe a func injector.
 		if !hasFuncInjector {

mvc/controller_test.go

@@ -433,7 +433,7 @@ func TestControllerActivateListener(t *testing.T) {
 	app := iris.New()
 	NewEngine().Controller(app, new(testControllerActivateListener))
 	m := NewEngine()
-	m.Dependencies.Add(&testBindType{ // will bind to all controllers under this .New() MVC Engine.
+	m.Dependencies.Add(&testBindType{
 		title: "my title",
 	})
 	m.Controller(app.Party("/manual"), new(testControllerActivateListener))
@@ -452,3 +452,38 @@ func TestControllerActivateListener(t *testing.T) {
 	e.GET("/manual2").Expect().Status(iris.StatusOK).
 		Body().Equal("my title")
 }
+
+type testControllerNotCreateNewDueManuallySettingAllFields struct {
+	TitlePointer *testBindType
+}
+
+func (c *testControllerNotCreateNewDueManuallySettingAllFields) Get() string {
+	return c.TitlePointer.title
+}
+
+func TestControllerNotCreateNewDueManuallySettingAllFields(t *testing.T) {
+	app := iris.New()
+	NewEngine().Controller(app, &testControllerNotCreateNewDueManuallySettingAllFields{
+		TitlePointer: &testBindType{
+			title: "my title",
+		},
+	}, func(ca *ControllerActivator) {
+		if n := len(ca.Dependencies.Values); n != 1 {
+			t.Fatalf(`expecting 1 dependency, the 'TitlePointer' which we manually insert
+				and the fields length is 1 so it will not create a new controller on each request
+				however the dependencies are available here
+				although the struct injector is being ignored when
+				creating the controller's handlers because we set it to invalidate state at "newControllerActivator"
+				--  got dependencies length: %d`, n)
+		}
+
+		if ca.IsRequestScoped() {
+			t.Fatalf(`this controller shouldn't be tagged used as request scoped(create new instances on each request),
+			 it doesn't contain any dynamic value or dependencies that should be binded via the iris mvc engine`)
+		}
+	})
+
+	e := httptest.New(t, app)
+	e.GET("/").Expect().Status(iris.StatusOK).
+		Body().Equal("my title")
+}

mvc/di/struct.go

@@ -15,8 +15,7 @@ type (
 		elemType reflect.Type
 		//
 		fields []*targetStructField
-		Valid  bool // is True when contains fields and it's a valid target struct.
+		Valid  bool   // is true when contains fields and it's a valid target struct.
-
 		trace  string // for debug info.
 	}
 )

mvc/func_result_test.go

@@ -178,7 +178,7 @@ func TestControllerMethodResultTypes(t *testing.T) {
 	app := iris.New()
 	NewEngine().Controller(app, new(testControllerMethodResultTypes))
 
-	e := httptest.New(t, app, httptest.LogLevel("debug"))
+	e := httptest.New(t, app)
 
 	e.GET("/text").Expect().Status(iris.StatusOK).
 		Body().Equal("text")