From 5d25a7bf2b25c233b6ce092093b6b99f395d1c0b Mon Sep 17 00:00:00 2001 From: Joshua Zhou Date: Fri, 2 Aug 2024 22:07:47 -0400 Subject: [PATCH 1/5] Add nonunique error --- errors.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/errors.go b/errors.go index 90fa714..197ca0c 100644 --- a/errors.go +++ b/errors.go @@ -37,6 +37,10 @@ var ( // ErrRelationshipMissingRequiredMembers indicates that a relationship does not have at least one required member ErrRelationshipMissingRequiredMembers = errors.New("relationship is missing required top-level members; must have one of: \"data\", \"meta\", \"links\"") + // ErrNonuniqueResource indicates that multiple resource objects across the primary data and included sections share + // the same type & id, or multiple resource linkages with the same type & id exist in a relationship section + ErrNonuniqueResource = errors.New("\"type\" and \"id\" must be unique across resources") + // ErrErrorUnmarshalingNotImplemented indicates that an attempt was made to unmarshal an error document ErrErrorUnmarshalingNotImplemented = errors.New("error unmarshaling is not implemented") ) From b60cfa907cb2ea1adf3c2fcdc3ef7eb09ee7732b Mon Sep 17 00:00:00 2001 From: Joshua Zhou Date: Fri, 2 Aug 2024 22:21:50 -0400 Subject: [PATCH 2/5] Add logic for checking uniqueness --- jsonapi.go | 68 +++++++++++++++++++++++++++++++++++++--------------- marshal.go | 3 +++ unmarshal.go | 5 +++- 3 files changed, 56 insertions(+), 20 deletions(-) diff --git a/jsonapi.go b/jsonapi.go index 9dac84d..188e140 100644 --- a/jsonapi.go +++ b/jsonapi.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "reflect" + "slices" ) // ResourceObject is a JSON:API resource object as defined by https://jsonapi.org/format/1.0/#document-resource-objects @@ -44,6 +45,10 @@ func (ro *resourceObject) UnmarshalJSON(data []byte) error { return nil } +func (ro *resourceObject) getIdentifier() string { + return fmt.Sprintf("{Type: %v, ID: %v}", ro.Type, ro.ID) +} + // JSONAPI is a JSON:API object as defined by https://jsonapi.org/format/1.0/#document-jsonapi-object. type jsonAPI struct { Version string `json:"version"` @@ -235,6 +240,16 @@ func (d *document) isEmpty() bool { return len(d.DataMany) == 0 && d.DataOne == nil } +func (d *document) getResourceObjectSlice() []*resourceObject { + if d.hasMany { + return d.DataMany + } + if d.DataOne == nil { + return nil + } + return []*resourceObject{d.DataOne} +} + // verifyFullLinkage returns an error if the given compound document is not fully-linked as // described by https://jsonapi.org/format/1.1/#document-compound-documents. That is, there must be // a chain of relationships linking all included data to primary data transitively. @@ -243,20 +258,6 @@ func (d *document) verifyFullLinkage(aliasRelationships bool) error { return nil } - getResourceObjectSlice := func(d *document) []*resourceObject { - if d.hasMany { - return d.DataMany - } - if d.DataOne == nil { - return nil - } - return []*resourceObject{d.DataOne} - } - - resourceIdentifier := func(ro *resourceObject) string { - return fmt.Sprintf("{Type: %v, ID: %v}", ro.Type, ro.ID) - } - // a list of related resource identifiers, and a flag to mark nodes as visited type includeNode struct { included *resourceObject @@ -270,16 +271,16 @@ func (d *document) verifyFullLinkage(aliasRelationships bool) error { relatedTo := make([]*resourceObject, 0) for _, relationship := range included.Relationships { - relatedTo = append(relatedTo, getResourceObjectSlice(relationship)...) + relatedTo = append(relatedTo, relationship.getResourceObjectSlice()...) } - includeGraph[resourceIdentifier(included)] = &includeNode{included, relatedTo, false} + includeGraph[included.getIdentifier()] = &includeNode{included, relatedTo, false} } // helper to traverse the graph from a given key and mark nodes as visited var visit func(ro *resourceObject) visit = func(ro *resourceObject) { - node, ok := includeGraph[resourceIdentifier(ro)] + node, ok := includeGraph[ro.getIdentifier()] if !ok { return } @@ -299,10 +300,10 @@ func (d *document) verifyFullLinkage(aliasRelationships bool) error { } // visit all include nodes that are accessible from the primary data - primaryData := getResourceObjectSlice(d) + primaryData := d.getResourceObjectSlice() for _, data := range primaryData { for _, relationship := range data.Relationships { - for _, ro := range getResourceObjectSlice(relationship) { + for _, ro := range relationship.getResourceObjectSlice() { visit(ro) } } @@ -322,6 +323,35 @@ func (d *document) verifyFullLinkage(aliasRelationships bool) error { return nil } +// verifyResourceUniqueness checks if the given document contains unique resources as described +// by https://jsonapi.org/format/1.1/#document-resource-object-identification. Resource objects +// across primary data and included must be unique, and resource linkages must be unique in +// any given relationship section. +func (d *document) verifyResourceUniqueness() bool { + topLevelSeen := make(map[string]struct{}) + + for _, ro := range slices.Concat(d.getResourceObjectSlice(), d.Included) { + rid := ro.getIdentifier() + if _, ok := topLevelSeen[rid]; ro.ID != "" && ok { + return false + } + topLevelSeen[rid] = struct{}{} + + relSeen := make(map[string]struct{}) + for _, rel := range ro.Relationships { + for _, relRo := range rel.getResourceObjectSlice() { + relRid := relRo.getIdentifier() + if _, ok := relSeen[relRid]; relRo.ID != "" && ok { + return false + } + relSeen[relRid] = struct{}{} + } + } + } + + return true +} + // Linkable can be implemented to marshal resource object links as defined by https://jsonapi.org/format/1.0/#document-resource-object-links. type Linkable interface { Link() *Link diff --git a/marshal.go b/marshal.go index 167441e..48525f9 100644 --- a/marshal.go +++ b/marshal.go @@ -205,6 +205,9 @@ func makeDocument(v any, m *Marshaler, isRelationship bool) (*document, error) { d.Included = append(d.Included, ro) } + if ok := d.verifyResourceUniqueness(); !ok { + return nil, ErrNonuniqueResource + } // if we got any included data, verify full-linkage of this compound document. if err := d.verifyFullLinkage(false); err != nil { return nil, err diff --git a/unmarshal.go b/unmarshal.go index c124b84..722b09a 100644 --- a/unmarshal.go +++ b/unmarshal.go @@ -88,6 +88,9 @@ func Unmarshal(data []byte, v any, opts ...UnmarshalOption) (err error) { } func (d *document) unmarshal(v any, m *Unmarshaler) (err error) { + if ok := d.verifyResourceUniqueness(); !ok { + return ErrNonuniqueResource + } // verify full-linkage in-case this is a compound document if err = d.verifyFullLinkage(true); err != nil { return @@ -142,7 +145,7 @@ func unmarshalResourceObjects(ros []*resourceObject, v any, m *Unmarshaler) erro outType := derefType(reflect.TypeOf(v)) outValue := derefValue(reflect.ValueOf(v)) - // first, it must be a struct since we'll be parsing the jsonapi struct tags + // first, it must be a slice since we'll be parsing multiple resource objects if outType.Kind() != reflect.Slice { return &TypeError{Actual: outType.String(), Expected: []string{"slice"}} } From 7ffc42bc02d8644b38ae5785585ba42a33b2c894 Mon Sep 17 00:00:00 2001 From: Joshua Zhou Date: Fri, 2 Aug 2024 22:23:46 -0400 Subject: [PATCH 3/5] Add tests --- jsonapi_test.go | 22 +++++++++++++--------- marshal_test.go | 13 ++++++++++++- unmarshal_test.go | 22 +++++++++++++++++++++- 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/jsonapi_test.go b/jsonapi_test.go index bdade8c..f9bcd24 100644 --- a/jsonapi_test.go +++ b/jsonapi_test.go @@ -31,6 +31,7 @@ var ( articleANoID = Article{Title: "A"} articleB = Article{ID: "2", Title: "B"} articlesAB = []Article{articleA, articleB} + articlesAA = []Article{articleA, articleA} articlesABPtr = []*Article{&articleA, &articleB} articleComplete = ArticleComplete{ ID: "1", @@ -77,15 +78,16 @@ var ( articleWithoutResourceObjectMeta = ArticleWithResourceObjectMeta{ID: "1", Title: "A"} // articles with relationships - articleRelated = ArticleRelated{ID: "1", Title: "A"} - articleRelatedNoOmitEmpty = ArticleRelatedNoOmitEmpty{ID: "1", Title: "A"} - articleRelatedAuthor = ArticleRelated{ID: "1", Title: "A", Author: &authorA} - articleRelatedAuthorWithMeta = ArticleRelated{ID: "1", Title: "A", Author: &authorAWithMeta} - articleRelatedComments = ArticleRelated{ID: "1", Title: "A", Comments: []*Comment{&commentA}} - articleRelatedCommentsArchived = ArticleRelated{ID: "1", Title: "A", Comments: []*Comment{&commentArchived}} - articleRelatedCommentsNested = ArticleRelated{ID: "1", Title: "A", Comments: []*Comment{&commentAWithAuthor}} - articleRelatedComplete = ArticleRelated{ID: "1", Title: "A", Author: &authorAWithMeta, Comments: commentsAB} - articlesRelatedComplex = []*ArticleRelated{ + articleRelated = ArticleRelated{ID: "1", Title: "A"} + articleRelatedNoOmitEmpty = ArticleRelatedNoOmitEmpty{ID: "1", Title: "A"} + articleRelatedAuthor = ArticleRelated{ID: "1", Title: "A", Author: &authorA} + articleRelatedAuthorWithMeta = ArticleRelated{ID: "1", Title: "A", Author: &authorAWithMeta} + articleRelatedComments = ArticleRelated{ID: "1", Title: "A", Comments: []*Comment{&commentA}} + articleRelatedNonuniqueComments = ArticleRelated{ID: "1", Title: "A", Comments: []*Comment{&commentA, &commentA}} + articleRelatedCommentsArchived = ArticleRelated{ID: "1", Title: "A", Comments: []*Comment{&commentArchived}} + articleRelatedCommentsNested = ArticleRelated{ID: "1", Title: "A", Comments: []*Comment{&commentAWithAuthor}} + articleRelatedComplete = ArticleRelated{ID: "1", Title: "A", Author: &authorAWithMeta, Comments: commentsAB} + articlesRelatedComplex = []*ArticleRelated{ { ID: "1", Title: "Bazel 101", @@ -153,6 +155,7 @@ var ( articleOmitTitleFullBody = `{"data":{"type":"articles","id":"1"}}` articleOmitTitlePartialBody = `{"data":{"type":"articles","id":"1","attributes":{"subtitle":"A"}}}` articlesABBody = `{"data":[{"type":"articles","id":"1","attributes":{"title":"A"}},{"type":"articles","id":"2","attributes":{"title":"B"}}]}` + articlesABNonuniqueData = `{"data":[{"type":"articles","id":"1","attributes":{"title":"A"}},{"type":"articles","id":"1","attributes":{"title":"B"}}]}` articleCompleteBody = `{"data":{"id":"1","type":"articles","attributes":{"info":{"publishDate":"1989-06-15T00:00:00Z","tags":["a","b"],"isPublic":true,"metrics":{"views":10,"reads":4}},"title":"A","subtitle":"AA"}}}` articleALinkedBody = `{"data":{"id":"1","type":"articles","attributes":{"title":"A"},"links":{"self":"https://example.com/articles/1","related":{"href":"https://example.com/articles/1/comments","meta":{"count":10}}}}}` articleLinkedOnlySelfBody = `{"data":{"id":"1","type":"articles","links":{"self":"https://example.com/articles/1"}}}` @@ -178,6 +181,7 @@ var ( articleRelatedCommentsWithIncludeBody = `{"data":{"id":"1","type":"articles","attributes":{"title":"A"},"relationships":{"comments":{"data":[{"id":"1","type":"comments"}],"links":{"self":"http://example.com/articles/1/relationships/comments","related":"http://example.com/articles/1/comments"}}}},"included":[{"id":"1","type":"comments","attributes":{"body":"A"},"relationships":{"author":{"data":{"id":"1","type":"author"},"links":{"self":"http://example.com/comments/1/relationships/author","related":"http://example.com/comments/1/author"}}}}]}` articleRelatedCompleteBody = `{"data":{"id":"1","type":"articles","attributes":{"title":"A"},"relationships":{"author":{"data":{"id":"1","type":"author"},"meta":{"count":10},"links":{"self":"http://example.com/articles/1/relationships/author","related":"http://example.com/articles/1/author"}},"comments":{"data":[{"id":"1","type":"comments"},{"id":"2","type":"comments"}],"links":{"self":"http://example.com/articles/1/relationships/comments","related":"http://example.com/articles/1/comments"}}}}}` articleRelatedCompleteWithIncludeBody = `{"data":{"id":"1","type":"articles","attributes":{"title":"A"},"relationships":{"author":{"data":{"id":"1","type":"author"}},"comments":{"data":[{"id":"1","type":"comments"},{"id":"2","type":"comments"}]}}},"included":[{"id":"1","type":"author","attributes":{"name":"A"}},{"id":"1","type":"comments","attributes":{"body":"A"}},{"id":"2","type":"comments","attributes":{"body":"B"}}]}` + articleRelatedNonuniqueLinkage = `{"data":{"id":"1","type":"articles","attributes":{"title":"A"},"relationships":{"author":{"data":{"id":"1","type":"author"}},"comments":{"data":[{"id":"1","type":"comments"},{"id":"1","type":"comments"}]}}}}` articleRelatedCommentsNestedWithIncludeBody = `{"data":{"id":"1","type":"articles","attributes":{"title":"A"},"relationships":{"comments":{"data":[{"id":"1","type":"comments"}],"links":{"self":"http://example.com/articles/1/relationships/comments","related":"http://example.com/articles/1/comments"}}}},"included":[{"id":"1","type":"comments","attributes":{"body":"A"},"relationships":{"author":{"data":{"id":"1","type":"author"},"links":{"self":"http://example.com/comments/1/relationships/author","related":"http://example.com/comments/1/author"}}}},{"id":"1","type":"author","attributes":{"name":"A"}}]}` articleWithIncludeOnlyBody = `{"data":{"id":"1","type":"articles","attributes":{"title":"A"}},"included":[{"id":"1","type":"author","attributes":{"name":"A"}}]}` articleRelatedAuthorLinksOnlyBody = `{"data":{"id":"1","type":"articles","attributes":{"title":"A"},"relationships":{"author":{"links":{"self":"http://example.com/articles/1/relationships/author","related":"http://example.com/articles/1/author"}}}}}` diff --git a/marshal_test.go b/marshal_test.go index 2a2a86b..ff1d248 100644 --- a/marshal_test.go +++ b/marshal_test.go @@ -81,6 +81,11 @@ func TestMarshal(t *testing.T) { given: &articlesAB, expect: articlesABBody, expectError: nil, + }, { + description: "[]Article (nonunique data)", + given: articlesAA, + expect: "", + expectError: ErrNonuniqueResource, }, { description: "[]*Article", given: articlesABPtr, @@ -584,6 +589,12 @@ func TestMarshalRelationships(t *testing.T) { marshalOptions: nil, expect: articleRelatedCommentsBody, expectError: nil, + }, { + description: "with related nonunique comments", + given: &articleRelatedNonuniqueComments, + marshalOptions: nil, + expect: "", + expectError: ErrNonuniqueResource, }, { description: "with related author and comments", given: &articleRelatedComplete, @@ -752,7 +763,7 @@ func TestMarshalMemberNameValidation(t *testing.T) { }, { description: "Articles with one invalid resource meta member name", given: []*ArticleWithGenericMeta{ - {ID: "1"}, {ID: "1", Meta: map[string]any{"foo%": 1}}, + {ID: "1"}, {ID: "2", Meta: map[string]any{"foo%": 1}}, }, expectError: &MemberNameValidationError{"foo%"}, }, { diff --git a/unmarshal_test.go b/unmarshal_test.go index 7b8537c..6c29eaf 100644 --- a/unmarshal_test.go +++ b/unmarshal_test.go @@ -69,6 +69,16 @@ func TestUnmarshal(t *testing.T) { }, expect: []Article{articleA, articleB}, expectError: nil, + }, { + description: "[]Article (nonunique data)", + given: articlesABNonuniqueData, + do: func(body []byte) (any, error) { + var a []Article + err := Unmarshal(body, &a) + return a, err + }, + expect: ([]Article)(nil), + expectError: ErrNonuniqueResource, }, { description: "[]Article (empty)", given: emptyManyBody, @@ -415,6 +425,16 @@ func TestUnmarshal(t *testing.T) { }, expect: &articleRelatedCommentsNested, expectError: nil, + }, { + description: "ArticleRelated (nonunique linkage)", + given: articleRelatedNonuniqueLinkage, + do: func(body []byte) (any, error) { + var a ArticleRelated + err := Unmarshal(body, &a) + return a, err + }, + expect: ArticleRelated{}, + expectError: ErrNonuniqueResource, }, { description: "links member only", given: `{"links":null}`, @@ -712,7 +732,7 @@ func TestUnmarshalMemberNameValidation(t *testing.T) { articleWithInvalidToplevelMetaMemberNameBody := `{"data":{"id":"1","type":"articles","attributes":{"title":"A"}},"meta":{"foo%":2}}` articleWithInvalidJSONAPIMetaMemberNameBody := `{"data":{"id":"1","type":"articles","attributes":{"title":"A"}},"jsonapi":{"version":"1.0","meta":{"foo%":1}}}` articleWithInvalidRelationshipAttributeNameNotIncludedBody := `{"data":{"id":"1","type":"articles","relationships":{"author":{"data":{"id":"1","type":"author"}}}}}` - articlesWithOneInvalidResourceMetaMemberName := `{"data":[{"id":"1","type":"articles"},{"id":"1","type":"articles","meta":{"foo%":1}}]}` + articlesWithOneInvalidResourceMetaMemberName := `{"data":[{"id":"1","type":"articles"},{"id":"2","type":"articles","meta":{"foo%":1}}]}` tests := []struct { description string From 2f4bc8f20432f6e502e73681c01feef0dbda4ee7 Mon Sep 17 00:00:00 2001 From: Joshua Zhou Date: Fri, 2 Aug 2024 23:02:49 -0400 Subject: [PATCH 4/5] Remove slices dependency (wrong go version) --- jsonapi.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/jsonapi.go b/jsonapi.go index 188e140..7ec8dc0 100644 --- a/jsonapi.go +++ b/jsonapi.go @@ -5,7 +5,6 @@ import ( "encoding/json" "fmt" "reflect" - "slices" ) // ResourceObject is a JSON:API resource object as defined by https://jsonapi.org/format/1.0/#document-resource-objects @@ -330,7 +329,7 @@ func (d *document) verifyFullLinkage(aliasRelationships bool) error { func (d *document) verifyResourceUniqueness() bool { topLevelSeen := make(map[string]struct{}) - for _, ro := range slices.Concat(d.getResourceObjectSlice(), d.Included) { + for _, ro := range append(d.getResourceObjectSlice(), d.Included...) { rid := ro.getIdentifier() if _, ok := topLevelSeen[rid]; ro.ID != "" && ok { return false From cdfce722c72350132be3549c8b6dd06ec8c36b4a Mon Sep 17 00:00:00 2001 From: Joshua Zhou Date: Wed, 7 Aug 2024 23:20:20 -0400 Subject: [PATCH 5/5] Nit change map value to bool for sets --- jsonapi.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/jsonapi.go b/jsonapi.go index 7ec8dc0..83e1cab 100644 --- a/jsonapi.go +++ b/jsonapi.go @@ -327,23 +327,23 @@ func (d *document) verifyFullLinkage(aliasRelationships bool) error { // across primary data and included must be unique, and resource linkages must be unique in // any given relationship section. func (d *document) verifyResourceUniqueness() bool { - topLevelSeen := make(map[string]struct{}) + topLevelSeen := make(map[string]bool) for _, ro := range append(d.getResourceObjectSlice(), d.Included...) { rid := ro.getIdentifier() - if _, ok := topLevelSeen[rid]; ro.ID != "" && ok { + if ro.ID != "" && topLevelSeen[rid] { return false } - topLevelSeen[rid] = struct{}{} + topLevelSeen[rid] = true - relSeen := make(map[string]struct{}) + relSeen := make(map[string]bool) for _, rel := range ro.Relationships { for _, relRo := range rel.getResourceObjectSlice() { relRid := relRo.getIdentifier() - if _, ok := relSeen[relRid]; relRo.ID != "" && ok { + if relRo.ID != "" && relSeen[relRid] { return false } - relSeen[relRid] = struct{}{} + relSeen[relRid] = true } } }