Skip to content

Commit 80e11a9

Browse files
Maed223Maed223
authored
Merge pull request #32 from hashicorp/TF-22988/circular-inclusions-stackoverflow
Bug Fix: Avoid Stack Overflow when provided data model relationships are included and contain circular references
2 parents fae13ce + 0f43dd8 commit 80e11a9

File tree

3 files changed

+200
-23
lines changed

3 files changed

+200
-23
lines changed

‎models_test.go

+29-8
Original file line numberDiff line numberDiff line change
@@ -89,15 +89,36 @@ type GenericInterface struct {
8989
Data interface{} `jsonapi:"attr,interface"`
9090
}
9191

92+
type Organization struct {
93+
ID int `jsonapi:"primary,organizations"`
94+
ClientID string `jsonapi:"client-id"`
95+
Name string `jsonapi:"attr,title"`
96+
DefaultProject *Project `jsonapi:"relation,default_project"`
97+
CreatedAt time.Time `jsonapi:"attr,created_at"`
98+
99+
Links Links `jsonapi:"links,omitempty"`
100+
}
101+
102+
type Project struct {
103+
ID int `jsonapi:"primary,projects"`
104+
ClientID string `jsonapi:"client-id"`
105+
Name string `jsonapi:"attr,name"`
106+
Organization *Organization `jsonapi:"relation,organization"`
107+
108+
Links Links `jsonapi:"links,omitempty"`
109+
}
110+
92111
type Blog struct {
93-
ID int `jsonapi:"primary,blogs"`
94-
ClientID string `jsonapi:"client-id"`
95-
Title string `jsonapi:"attr,title"`
96-
Posts []*Post `jsonapi:"relation,posts"`
97-
CurrentPost *Post `jsonapi:"relation,current_post"`
98-
CurrentPostID int `jsonapi:"attr,current_post_id"`
99-
CreatedAt time.Time `jsonapi:"attr,created_at"`
100-
ViewCount int `jsonapi:"attr,view_count"`
112+
ID int `jsonapi:"primary,blogs"`
113+
ClientID string `jsonapi:"client-id"`
114+
Title string `jsonapi:"attr,title"`
115+
CurrentPostID int `jsonapi:"attr,current_post_id"`
116+
CreatedAt time.Time `jsonapi:"attr,created_at"`
117+
ViewCount int `jsonapi:"attr,view_count"`
118+
Posts []*Post `jsonapi:"relation,posts"`
119+
CurrentPost *Post `jsonapi:"relation,current_post"`
120+
Organization *Organization `jsonapi:"relation,organization"`
121+
Project *Project `jsonapi:"relation,project"`
101122

102123
Links Links `jsonapi:"links,omitempty"`
103124
}

‎request.go

+37-15
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,11 @@ func newErrUnsupportedPtrType(rf reflect.Value, t reflect.Type, structField refl
6060
return ErrUnsupportedPtrType{rf, t, structField}
6161
}
6262

63+
type includedNode struct {
64+
node *Node
65+
model *reflect.Value
66+
}
67+
6368
// UnmarshalPayload converts an io into a struct instance using jsonapi tags on
6469
// struct fields. This method supports single request payloads only, at the
6570
// moment. Bulk creates and updates are not supported yet.
@@ -94,19 +99,19 @@ func newErrUnsupportedPtrType(rf reflect.Value, t reflect.Type, structField refl
9499
// model interface{} should be a pointer to a struct.
95100
func UnmarshalPayload(in io.Reader, model interface{}) error {
96101
payload := new(OnePayload)
102+
included := make(map[string]*includedNode)
97103

98104
if err := json.NewDecoder(in).Decode(payload); err != nil {
99105
return err
100106
}
101107

102108
if payload.Included != nil {
103-
includedMap := make(map[string]*Node)
104-
for _, included := range payload.Included {
105-
key := fmt.Sprintf("%s,%s", included.Type, included.ID)
106-
includedMap[key] = included
109+
for _, include := range payload.Included {
110+
key := fmt.Sprintf("%s,%s", include.Type, include.ID)
111+
included[key] = &includedNode{include, nil}
107112
}
108113

109-
return unmarshalNode(payload.Data, reflect.ValueOf(model), &includedMap)
114+
return unmarshalNode(payload.Data, reflect.ValueOf(model), &included)
110115
}
111116
return unmarshalNode(payload.Data, reflect.ValueOf(model), nil)
112117
}
@@ -120,19 +125,19 @@ func UnmarshalManyPayload(in io.Reader, t reflect.Type) ([]interface{}, error) {
120125
return nil, err
121126
}
122127

123-
models := []interface{}{} // will be populated from the "data"
124-
includedMap := map[string]*Node{} // will be populate from the "included"
128+
models := []interface{}{} // will be populated from the "data"
129+
included := map[string]*includedNode{} // will be populate from the "included"
125130

126131
if payload.Included != nil {
127-
for _, included := range payload.Included {
128-
key := fmt.Sprintf("%s,%s", included.Type, included.ID)
129-
includedMap[key] = included
132+
for _, include := range payload.Included {
133+
key := fmt.Sprintf("%s,%s", include.Type, include.ID)
134+
included[key] = &includedNode{include, nil}
130135
}
131136
}
132137

133138
for _, data := range payload.Data {
134139
model := reflect.New(t.Elem())
135-
err := unmarshalNode(data, model, &includedMap)
140+
err := unmarshalNode(data, model, &included)
136141
if err != nil {
137142
return nil, err
138143
}
@@ -263,7 +268,7 @@ func getStructTags(field reflect.StructField) ([]string, error) {
263268

264269
// unmarshalNodeMaybeChoice populates a model that may or may not be
265270
// a choice type struct that corresponds to a polyrelation or relation
266-
func unmarshalNodeMaybeChoice(m *reflect.Value, data *Node, annotation string, choiceTypeMapping map[string]structFieldIndex, included *map[string]*Node) error {
271+
func unmarshalNodeMaybeChoice(m *reflect.Value, data *Node, annotation string, choiceTypeMapping map[string]structFieldIndex, included *map[string]*includedNode) error {
267272
// This will hold either the value of the choice type model or the actual
268273
// model, depending on annotation
269274
var actualModel = *m
@@ -300,7 +305,7 @@ func unmarshalNodeMaybeChoice(m *reflect.Value, data *Node, annotation string, c
300305
return nil
301306
}
302307

303-
func unmarshalNode(data *Node, model reflect.Value, included *map[string]*Node) (err error) {
308+
func unmarshalNode(data *Node, model reflect.Value, included *map[string]*includedNode) (err error) {
304309
defer func() {
305310
if r := recover(); r != nil {
306311
err = fmt.Errorf("data is not a jsonapi representation of '%v'", model.Type())
@@ -509,6 +514,23 @@ func unmarshalNode(data *Node, model reflect.Value, included *map[string]*Node)
509514
// model, depending on annotation
510515
m := reflect.New(fieldValue.Type().Elem())
511516

517+
// Check if the item in the relationship was already processed elsewhere. Avoids potential infinite recursive loops
518+
// caused by circular references between included relationships (two included items include one another)
519+
includedKey := fmt.Sprintf("%s,%s", relationship.Data.Type, relationship.Data.ID)
520+
if included != nil && (*included)[includedKey] != nil {
521+
if (*included)[includedKey].model != nil {
522+
fieldValue.Set(*(*included)[includedKey].model)
523+
} else {
524+
(*included)[includedKey].model = &m
525+
err := unmarshalNodeMaybeChoice(&m, (*included)[includedKey].node, annotation, choiceMapping, included)
526+
if err != nil {
527+
er = err
528+
break
529+
}
530+
fieldValue.Set(m)
531+
}
532+
continue
533+
}
512534
err = unmarshalNodeMaybeChoice(&m, relationship.Data, annotation, choiceMapping, included)
513535
if err != nil {
514536
er = err
@@ -565,11 +587,11 @@ func unmarshalNode(data *Node, model reflect.Value, included *map[string]*Node)
565587
return er
566588
}
567589

568-
func fullNode(n *Node, included *map[string]*Node) *Node {
590+
func fullNode(n *Node, included *map[string]*includedNode) *Node {
569591
includedKey := fmt.Sprintf("%s,%s", n.Type, n.ID)
570592

571593
if included != nil && (*included)[includedKey] != nil {
572-
return (*included)[includedKey]
594+
return (*included)[includedKey].node
573595
}
574596

575597
return n

‎request_test.go

+134
Original file line numberDiff line numberDiff line change
@@ -689,6 +689,41 @@ func TestUnmarshalRelationships(t *testing.T) {
689689
}
690690
}
691691

692+
func TestUnmarshalMany_relationships_with_circular_inclusion(t *testing.T) {
693+
data := samplePayloadWithCircularInclusion()
694+
payload, err := json.Marshal(data)
695+
if err != nil {
696+
t.Fatal(err)
697+
}
698+
in := bytes.NewReader(payload)
699+
model := reflect.TypeOf(new(Blog))
700+
701+
out, err := UnmarshalManyPayload(in, model)
702+
if err != nil {
703+
t.Fatal(err)
704+
}
705+
706+
result_1 := out[0].(*Blog)
707+
708+
if result_1.Project != result_1.Organization.DefaultProject {
709+
t.Errorf("expected blog.project (%p) to hold the same pointer as blog.organization.default-project (%p) ", result_1.Project, result_1.Organization.DefaultProject)
710+
}
711+
712+
if result_1.Organization != result_1.Project.Organization {
713+
t.Errorf("expected blog.organization (%p) to hold the same pointer as blog.project.organization (%p)", result_1.Organization, result_1.Project.Organization)
714+
}
715+
716+
result_2 := out[1].(*Blog)
717+
718+
if result_2.Project != result_2.Organization.DefaultProject {
719+
t.Errorf("expected blog.project (%p) to hold the same pointer as blog.organization.default-project (%p) ", result_2.Project, result_2.Organization.DefaultProject)
720+
}
721+
722+
if result_2.Organization != result_2.Project.Organization {
723+
t.Errorf("expected blog.organization (%p) to hold the same pointer as blog.project.organization (%p)", result_2.Organization, result_2.Project.Organization)
724+
}
725+
}
726+
692727
func Test_UnmarshalPayload_polymorphicRelations(t *testing.T) {
693728
in := bytes.NewReader([]byte(`{
694729
"data": {
@@ -1378,6 +1413,105 @@ func TestUnmarshalCustomTypeAttributes_ErrInvalidType(t *testing.T) {
13781413
}
13791414
}
13801415

1416+
func samplePayloadWithCircularInclusion() *ManyPayload {
1417+
payload := &ManyPayload{
1418+
Data: []*Node{
1419+
{
1420+
Type: "blogs",
1421+
ClientID: "1",
1422+
ID: "1",
1423+
Attributes: map[string]interface{}{
1424+
"title": "Foo",
1425+
"current_post_id": 1,
1426+
"created_at": 1436216820,
1427+
"view_count": 1000,
1428+
},
1429+
Relationships: map[string]interface{}{
1430+
"project": &RelationshipOneNode{
1431+
Data: &Node{
1432+
Type: "projects",
1433+
ClientID: "1",
1434+
ID: "1",
1435+
},
1436+
},
1437+
"organization": &RelationshipOneNode{
1438+
Data: &Node{
1439+
Type: "organizations",
1440+
ClientID: "1",
1441+
ID: "1",
1442+
},
1443+
},
1444+
},
1445+
},
1446+
{
1447+
Type: "blogs",
1448+
ClientID: "2",
1449+
ID: "2",
1450+
Attributes: map[string]interface{}{
1451+
"title": "Foo2",
1452+
"current_post_id": 1,
1453+
"created_at": 1436216820,
1454+
"view_count": 1000,
1455+
},
1456+
Relationships: map[string]interface{}{
1457+
"project": &RelationshipOneNode{
1458+
Data: &Node{
1459+
Type: "projects",
1460+
ClientID: "1",
1461+
ID: "1",
1462+
},
1463+
},
1464+
"organization": &RelationshipOneNode{
1465+
Data: &Node{
1466+
Type: "organizations",
1467+
ClientID: "1",
1468+
ID: "1",
1469+
},
1470+
},
1471+
},
1472+
},
1473+
},
1474+
Included: []*Node{
1475+
{
1476+
Type: "projects",
1477+
ClientID: "1",
1478+
ID: "1",
1479+
Attributes: map[string]interface{}{
1480+
"name": "Bar",
1481+
},
1482+
Relationships: map[string]interface{}{
1483+
"organization": &RelationshipOneNode{
1484+
Data: &Node{
1485+
Type: "organizations",
1486+
ClientID: "1",
1487+
ID: "1",
1488+
},
1489+
},
1490+
},
1491+
},
1492+
{
1493+
Type: "organizations",
1494+
ClientID: "1",
1495+
ID: "1",
1496+
Attributes: map[string]interface{}{
1497+
"name": "Baz",
1498+
},
1499+
Relationships: map[string]interface{}{
1500+
"default_project": &RelationshipOneNode{
1501+
Data: &Node{
1502+
Type: "projects",
1503+
ClientID: "1",
1504+
ID: "1",
1505+
},
1506+
},
1507+
},
1508+
},
1509+
},
1510+
}
1511+
1512+
return payload
1513+
}
1514+
13811515
func samplePayloadWithoutIncluded() map[string]interface{} {
13821516
return map[string]interface{}{
13831517
"data": map[string]interface{}{

0 commit comments

Comments
 (0)