Merge pull request #33 from hashicorp/brandonc/revert_circular_relationship_handling

Revert circular relationship handling

Author: brandonc

.github/CODEOWNERS

@@ -2,8 +2,8 @@
 # More on CODEOWNERS files: https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners
 
 # Default owners
-* @hashicorp/tf-core-cloud
 * @hashicorp/team-ip-compliance
+* @hashicorp/tf-core-cloud
 
 # Add override rules below. Each line is a file/folder pattern followed by one or more owners.
 # Being an owner means those groups or individuals will be added as reviewers to PRs affecting

models_test.go

@@ -90,36 +90,15 @@ type GenericInterface struct {
 	Data interface{} `jsonapi:"attr,interface"`
 }
 
-type Organization struct {
-	ID             int       `jsonapi:"primary,organizations"`
-	ClientID       string    `jsonapi:"client-id"`
-	Name           string    `jsonapi:"attr,title"`
-	DefaultProject *Project  `jsonapi:"relation,default_project"`
-	CreatedAt      time.Time `jsonapi:"attr,created_at"`
-
-	Links Links `jsonapi:"links,omitempty"`
-}
-
-type Project struct {
-	ID           int           `jsonapi:"primary,projects"`
-	ClientID     string        `jsonapi:"client-id"`
-	Name         string        `jsonapi:"attr,name"`
-	Organization *Organization `jsonapi:"relation,organization"`
-
-	Links Links `jsonapi:"links,omitempty"`
-}
-
 type Blog struct {
-	ID            int           `jsonapi:"primary,blogs"`
-	ClientID      string        `jsonapi:"client-id"`
-	Title         string        `jsonapi:"attr,title"`
-	CurrentPostID int           `jsonapi:"attr,current_post_id"`
-	CreatedAt     time.Time     `jsonapi:"attr,created_at"`
-	ViewCount     int           `jsonapi:"attr,view_count"`
-	Posts         []*Post       `jsonapi:"relation,posts"`
-	CurrentPost   *Post         `jsonapi:"relation,current_post"`
-	Organization  *Organization `jsonapi:"relation,organization"`
-	Project       *Project      `jsonapi:"relation,project"`
+	ID            int       `jsonapi:"primary,blogs"`
+	ClientID      string    `jsonapi:"client-id"`
+	Title         string    `jsonapi:"attr,title"`
+	Posts         []*Post   `jsonapi:"relation,posts"`
+	CurrentPost   *Post     `jsonapi:"relation,current_post"`
+	CurrentPostID int       `jsonapi:"attr,current_post_id"`
+	CreatedAt     time.Time `jsonapi:"attr,created_at"`
+	ViewCount     int       `jsonapi:"attr,view_count"`
 
 	Links Links `jsonapi:"links,omitempty"`
 }

request.go

@@ -60,11 +60,6 @@ func newErrUnsupportedPtrType(rf reflect.Value, t reflect.Type, structField refl
 	return ErrUnsupportedPtrType{rf, t, structField}
 }
 
-type includedNode struct {
-	node  *Node
-	model *reflect.Value
-}
-
 // UnmarshalPayload converts an io into a struct instance using jsonapi tags on
 // struct fields. This method supports single request payloads only, at the
 // moment. Bulk creates and updates are not supported yet.
@@ -99,19 +94,19 @@ type includedNode struct {
 // model interface{} should be a pointer to a struct.
 func UnmarshalPayload(in io.Reader, model interface{}) error {
 	payload := new(OnePayload)
-	included := make(map[string]*includedNode)
 
 	if err := json.NewDecoder(in).Decode(payload); err != nil {
 		return err
 	}
 
 	if payload.Included != nil {
-		for _, include := range payload.Included {
-			key := fmt.Sprintf("%s,%s", include.Type, include.ID)
-			included[key] = &includedNode{include, nil}
+		includedMap := make(map[string]*Node)
+		for _, included := range payload.Included {
+			key := fmt.Sprintf("%s,%s", included.Type, included.ID)
+			includedMap[key] = included
 		}
 
-		return unmarshalNode(payload.Data, reflect.ValueOf(model), &included)
+		return unmarshalNode(payload.Data, reflect.ValueOf(model), &includedMap)
 	}
 	return unmarshalNode(payload.Data, reflect.ValueOf(model), nil)
 }
@@ -125,19 +120,19 @@ func UnmarshalManyPayload(in io.Reader, t reflect.Type) ([]interface{}, error) {
 		return nil, err
 	}
 
-	models := []interface{}{}              // will be populated from the "data"
-	included := map[string]*includedNode{} // will be populate from the "included"
+	models := []interface{}{}         // will be populated from the "data"
+	includedMap := map[string]*Node{} // will be populate from the "included"
 
 	if payload.Included != nil {
-		for _, include := range payload.Included {
-			key := fmt.Sprintf("%s,%s", include.Type, include.ID)
-			included[key] = &includedNode{include, nil}
+		for _, included := range payload.Included {
+			key := fmt.Sprintf("%s,%s", included.Type, included.ID)
+			includedMap[key] = included
 		}
 	}
 
 	for _, data := range payload.Data {
 		model := reflect.New(t.Elem())
-		err := unmarshalNode(data, model, &included)
+		err := unmarshalNode(data, model, &includedMap)
 		if err != nil {
 			return nil, err
 		}
@@ -268,7 +263,7 @@ func getStructTags(field reflect.StructField) ([]string, error) {
 
 // unmarshalNodeMaybeChoice populates a model that may or may not be
 // a choice type struct that corresponds to a polyrelation or relation
-func unmarshalNodeMaybeChoice(m *reflect.Value, data *Node, annotation string, choiceTypeMapping map[string]structFieldIndex, included *map[string]*includedNode) error {
+func unmarshalNodeMaybeChoice(m *reflect.Value, data *Node, annotation string, choiceTypeMapping map[string]structFieldIndex, included *map[string]*Node) error {
 	// This will hold either the value of the choice type model or the actual
 	// model, depending on annotation
 	var actualModel = *m
@@ -305,7 +300,7 @@ func unmarshalNodeMaybeChoice(m *reflect.Value, data *Node, annotation string, c
 	return nil
 }
 
-func unmarshalNode(data *Node, model reflect.Value, included *map[string]*includedNode) (err error) {
+func unmarshalNode(data *Node, model reflect.Value, included *map[string]*Node) (err error) {
 	defer func() {
 		if r := recover(); r != nil {
 			err = fmt.Errorf("data is not a jsonapi representation of '%v'", model.Type())
@@ -534,23 +529,6 @@ func unmarshalNode(data *Node, model reflect.Value, included *map[string]*includ
 					continue
 				}
 
-				// Check if the item in the relationship was already processed elsewhere. Avoids potential infinite recursive loops
-				// caused by circular references between included relationships (two included items include one another)
-				includedKey := fmt.Sprintf("%s,%s", relationship.Data.Type, relationship.Data.ID)
-				if included != nil && (*included)[includedKey] != nil {
-					if (*included)[includedKey].model != nil {
-						fieldValue.Set(*(*included)[includedKey].model)
-					} else {
-						(*included)[includedKey].model = &m
-						err := unmarshalNodeMaybeChoice(&m, (*included)[includedKey].node, annotation, choiceMapping, included)
-						if err != nil {
-							er = err
-							break
-						}
-						fieldValue.Set(m)
-					}
-					continue
-				}
 				err = unmarshalNodeMaybeChoice(&m, relationship.Data, annotation, choiceMapping, included)
 				if err != nil {
 					er = err
@@ -612,11 +590,11 @@ func unmarshalNode(data *Node, model reflect.Value, included *map[string]*includ
 	return er
 }
 
-func fullNode(n *Node, included *map[string]*includedNode) *Node {
+func fullNode(n *Node, included *map[string]*Node) *Node {
 	includedKey := fmt.Sprintf("%s,%s", n.Type, n.ID)
 
 	if included != nil && (*included)[includedKey] != nil {
-		return (*included)[includedKey].node
+		return (*included)[includedKey]
 	}
 
 	return n

request_test.go

@@ -811,41 +811,6 @@ func TestUnmarshalRelationships(t *testing.T) {
 	}
 }
 
-func TestUnmarshalMany_relationships_with_circular_inclusion(t *testing.T) {
-	data := samplePayloadWithCircularInclusion()
-	payload, err := json.Marshal(data)
-	if err != nil {
-		t.Fatal(err)
-	}
-	in := bytes.NewReader(payload)
-	model := reflect.TypeOf(new(Blog))
-
-	out, err := UnmarshalManyPayload(in, model)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	result_1 := out[0].(*Blog)
-
-	if result_1.Project != result_1.Organization.DefaultProject {
-		t.Errorf("expected blog.project (%p) to hold the same pointer as blog.organization.default-project (%p) ", result_1.Project, result_1.Organization.DefaultProject)
-	}
-
-	if result_1.Organization != result_1.Project.Organization {
-		t.Errorf("expected blog.organization (%p) to hold the same pointer as blog.project.organization (%p)", result_1.Organization, result_1.Project.Organization)
-	}
-
-	result_2 := out[1].(*Blog)
-
-	if result_2.Project != result_2.Organization.DefaultProject {
-		t.Errorf("expected blog.project (%p) to hold the same pointer as blog.organization.default-project (%p) ", result_2.Project, result_2.Organization.DefaultProject)
-	}
-
-	if result_2.Organization != result_2.Project.Organization {
-		t.Errorf("expected blog.organization (%p) to hold the same pointer as blog.project.organization (%p)", result_2.Organization, result_2.Project.Organization)
-	}
-}
-
 func Test_UnmarshalPayload_polymorphicRelations(t *testing.T) {
 	in := bytes.NewReader([]byte(`{
 		"data": {
@@ -1535,105 +1500,6 @@ func TestUnmarshalCustomTypeAttributes_ErrInvalidType(t *testing.T) {
 	}
 }
 
-func samplePayloadWithCircularInclusion() *ManyPayload {
-	payload := &ManyPayload{
-		Data: []*Node{
-			{
-				Type:     "blogs",
-				ClientID: "1",
-				ID:       "1",
-				Attributes: map[string]interface{}{
-					"title":           "Foo",
-					"current_post_id": 1,
-					"created_at":      1436216820,
-					"view_count":      1000,
-				},
-				Relationships: map[string]interface{}{
-					"project": &RelationshipOneNode{
-						Data: &Node{
-							Type:     "projects",
-							ClientID: "1",
-							ID:       "1",
-						},
-					},
-					"organization": &RelationshipOneNode{
-						Data: &Node{
-							Type:     "organizations",
-							ClientID: "1",
-							ID:       "1",
-						},
-					},
-				},
-			},
-			{
-				Type:     "blogs",
-				ClientID: "2",
-				ID:       "2",
-				Attributes: map[string]interface{}{
-					"title":           "Foo2",
-					"current_post_id": 1,
-					"created_at":      1436216820,
-					"view_count":      1000,
-				},
-				Relationships: map[string]interface{}{
-					"project": &RelationshipOneNode{
-						Data: &Node{
-							Type:     "projects",
-							ClientID: "1",
-							ID:       "1",
-						},
-					},
-					"organization": &RelationshipOneNode{
-						Data: &Node{
-							Type:     "organizations",
-							ClientID: "1",
-							ID:       "1",
-						},
-					},
-				},
-			},
-		},
-		Included: []*Node{
-			{
-				Type:     "projects",
-				ClientID: "1",
-				ID:       "1",
-				Attributes: map[string]interface{}{
-					"name": "Bar",
-				},
-				Relationships: map[string]interface{}{
-					"organization": &RelationshipOneNode{
-						Data: &Node{
-							Type:     "organizations",
-							ClientID: "1",
-							ID:       "1",
-						},
-					},
-				},
-			},
-			{
-				Type:     "organizations",
-				ClientID: "1",
-				ID:       "1",
-				Attributes: map[string]interface{}{
-					"name": "Baz",
-				},
-				Relationships: map[string]interface{}{
-					"default_project": &RelationshipOneNode{
-						Data: &Node{
-							Type:     "projects",
-							ClientID: "1",
-							ID:       "1",
-						},
-					},
-				},
-			},
-		},
-	}
-
-	return payload
-}
-
 func samplePayloadWithoutIncluded() map[string]interface{} {
 	return map[string]interface{}{
 		"data": map[string]interface{}{