From 6faf18c2e887ee848eee3c28d035a15fc6c50559 Mon Sep 17 00:00:00 2001 From: HiteshRepo Date: Wed, 24 Jan 2024 15:17:21 +0530 Subject: [PATCH 1/6] provisions to identify fields with multiple value --- src/pkg/services/m365/api/consts.go | 3 + src/pkg/services/m365/api/lists.go | 53 +++++++++++---- src/pkg/services/m365/api/lists_test.go | 89 ++++++++++++++++++++++--- 3 files changed, 123 insertions(+), 22 deletions(-) diff --git a/src/pkg/services/m365/api/consts.go b/src/pkg/services/m365/api/consts.go index 96778c532a..676ce910f4 100644 --- a/src/pkg/services/m365/api/consts.go +++ b/src/pkg/services/m365/api/consts.go @@ -61,6 +61,9 @@ const ( ChildCountFieldNamePart = "ChildCount" LookupIDFieldNamePart = "LookupId" + ODataTypeFieldNamePart = "@odata.type" + ODataTypeFieldNameStringVal = "Collection(Edm.String)" + ReadOnlyOrHiddenFieldNamePrefix = "_" DescoratorFieldNamePrefix = "@" diff --git a/src/pkg/services/m365/api/lists.go b/src/pkg/services/m365/api/lists.go index 80493f4cea..cb9efe4ada 100644 --- a/src/pkg/services/m365/api/lists.go +++ b/src/pkg/services/m365/api/lists.go @@ -3,6 +3,7 @@ package api import ( "context" "fmt" + "reflect" "strings" "github.com/alcionai/clues" @@ -18,6 +19,10 @@ import ( var ErrSkippableListTemplate = clues.New("unable to create lists with skippable templates") +type columnDetails struct { + isMultipleEnabled bool +} + // --------------------------------------------------------------------------- // controller // --------------------------------------------------------------------------- @@ -277,7 +282,7 @@ func BytesToListable(bytes []byte) (models.Listable, error) { // not attached in this method. // ListItems are not included in creation of new list, and have to be restored // in separate call. -func ToListable(orig models.Listable, listName string) (models.Listable, map[string]any) { +func ToListable(orig models.Listable, listName string) (models.Listable, map[string]*columnDetails) { newList := models.NewList() newList.SetContentTypes(orig.GetContentTypes()) @@ -294,7 +299,7 @@ func ToListable(orig models.Listable, listName string) (models.Listable, map[str newList.SetParentReference(orig.GetParentReference()) columns := make([]models.ColumnDefinitionable, 0) - columnNames := map[string]any{TitleColumnName: nil} + columnNames := map[string]*columnDetails{TitleColumnName: {}} for _, cd := range orig.GetColumns() { var ( @@ -316,8 +321,7 @@ func ToListable(orig models.Listable, listName string) (models.Listable, map[str continue } - columns = append(columns, cloneColumnDefinitionable(cd)) - columnNames[ptr.Val(cd.GetName())] = nil + columns = append(columns, cloneColumnDefinitionable(cd, columnNames)) } newList.SetColumns(columns) @@ -327,7 +331,10 @@ func ToListable(orig models.Listable, listName string) (models.Listable, map[str // cloneColumnDefinitionable utility function for encapsulating models.ColumnDefinitionable data // into new object for upload. -func cloneColumnDefinitionable(orig models.ColumnDefinitionable) models.ColumnDefinitionable { +func cloneColumnDefinitionable( + orig models.ColumnDefinitionable, + columnNames map[string]*columnDetails, +) models.ColumnDefinitionable { newColumn := models.NewColumnDefinition() // column attributes @@ -351,7 +358,7 @@ func cloneColumnDefinitionable(orig models.ColumnDefinitionable) models.ColumnDe newColumn.SetEnforceUniqueValues(orig.GetEnforceUniqueValues()) // column types - setColumnType(newColumn, orig) + setColumnType(newColumn, orig, columnNames) // Requires nil checks to avoid Graph error: 'General exception while processing' defaultValue := orig.GetDefaultValue() @@ -367,7 +374,11 @@ func cloneColumnDefinitionable(orig models.ColumnDefinitionable) models.ColumnDe return newColumn } -func setColumnType(newColumn *models.ColumnDefinition, orig models.ColumnDefinitionable) { +func setColumnType( + newColumn *models.ColumnDefinition, + orig models.ColumnDefinitionable, + columnNames map[string]*columnDetails, +) { switch { case orig.GetText() != nil: newColumn.SetText(orig.GetText()) @@ -400,12 +411,14 @@ func setColumnType(newColumn *models.ColumnDefinition, orig models.ColumnDefinit default: newColumn.SetText(models.NewTextColumn()) } + + columnNames[ptr.Val(newColumn.GetName())] = &columnDetails{} } // CloneListItem creates a new `SharePoint.ListItem` and stores the original item's // M365 data into it set fields. // - https://learn.microsoft.com/en-us/graph/api/resources/listitem?view=graph-rest-1.0 -func CloneListItem(orig models.ListItemable, columnNames map[string]any) models.ListItemable { +func CloneListItem(orig models.ListItemable, columnNames map[string]*columnDetails) models.ListItemable { newItem := models.NewListItem() // list item data @@ -442,7 +455,7 @@ func CloneListItem(orig models.ListItemable, columnNames map[string]any) models. // additionalData map // Further documentation on FieldValueSets: // - https://learn.microsoft.com/en-us/graph/api/resources/fieldvalueset?view=graph-rest-1.0 -func retrieveFieldData(orig models.FieldValueSetable, columnNames map[string]any) models.FieldValueSetable { +func retrieveFieldData(orig models.FieldValueSetable, columnNames map[string]*columnDetails) models.FieldValueSetable { fields := models.NewFieldValueSet() additionalData := setAdditionalDataByColumnNames(orig, columnNames) @@ -463,7 +476,7 @@ func retrieveFieldData(orig models.FieldValueSetable, columnNames map[string]any func setAdditionalDataByColumnNames( orig models.FieldValueSetable, - columnNames map[string]any, + columnNames map[string]*columnDetails, ) map[string]any { if orig == nil { return make(map[string]any) @@ -472,15 +485,31 @@ func setAdditionalDataByColumnNames( fieldData := orig.GetAdditionalData() filteredData := make(map[string]any) - for colName := range columnNames { - if _, ok := fieldData[colName]; ok { + for colName, colDetails := range columnNames { + if val, ok := fieldData[colName]; ok { + setMultipleEnabled(val, colDetails) + filteredData[colName] = fieldData[colName] } + + specifyODataType(filteredData, colDetails, colName) } return filteredData } +func setMultipleEnabled(val any, colDetails *columnDetails) { + if reflect.TypeOf(val).Kind() == reflect.Slice { + colDetails.isMultipleEnabled = true + } +} + +func specifyODataType(filteredData map[string]any, colDetails *columnDetails, colName string) { + if colDetails.isMultipleEnabled { + filteredData[colName+ODataTypeFieldNamePart] = ODataTypeFieldNameStringVal + } +} + func hasAddressFields(additionalData map[string]any) (map[string]any, string, bool) { for k, v := range additionalData { nestedFields, ok := v.(map[string]any) diff --git a/src/pkg/services/m365/api/lists_test.go b/src/pkg/services/m365/api/lists_test.go index 100cb9288a..64dc0f1ecc 100644 --- a/src/pkg/services/m365/api/lists_test.go +++ b/src/pkg/services/m365/api/lists_test.go @@ -162,12 +162,12 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_GetValidation() { for _, test := range tests { suite.Run(test.name, func() { t := suite.T() + colNames := map[string]*columnDetails{} orig := test.getOrig() - newCd := cloneColumnDefinitionable(orig) + newCd := cloneColumnDefinitionable(orig, colNames) require.NotEmpty(t, newCd) - test.expect(t, newCd.GetValidation()) }) } @@ -219,9 +219,10 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_GetDefaultValue() { for _, test := range tests { suite.Run(test.name, func() { t := suite.T() + colNames := map[string]*columnDetails{} orig := test.getOrig() - newCd := cloneColumnDefinitionable(orig) + newCd := cloneColumnDefinitionable(orig, colNames) require.NotEmpty(t, newCd) test.expect(t, newCd) @@ -277,9 +278,8 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_ColumnType() { for _, test := range tests { suite.Run(test.name, func() { t := suite.T() - - orig := test.getOrig() - newCd := cloneColumnDefinitionable(orig) + colNames := map[string]*columnDetails{} + newCd := cloneColumnDefinitionable(test.getOrig(), colNames) require.NotEmpty(t, newCd) assert.True(t, test.checkFn(newCd)) @@ -432,7 +432,7 @@ func (suite *ListsUnitSuite) TestFieldValueSetable() { origFs := models.NewFieldValueSet() origFs.SetAdditionalData(additionalData) - colNames := map[string]any{} + colNames := map[string]*columnDetails{} fs := retrieveFieldData(origFs, colNames) fsAdditionalData := fs.GetAdditionalData() @@ -442,7 +442,7 @@ func (suite *ListsUnitSuite) TestFieldValueSetable() { origFs = models.NewFieldValueSet() origFs.SetAdditionalData(additionalData) - colNames["itemName"] = struct{}{} + colNames["itemName"] = &columnDetails{} fs = retrieveFieldData(origFs, colNames) fsAdditionalData = fs.GetAdditionalData() @@ -509,8 +509,8 @@ func (suite *ListsUnitSuite) TestFieldValueSetable_Location() { origFs := models.NewFieldValueSet() origFs.SetAdditionalData(additionalData) - colNames := map[string]any{ - "MyAddress": nil, + colNames := map[string]*columnDetails{ + "MyAddress": {}, } fs := retrieveFieldData(origFs, colNames) @@ -703,6 +703,75 @@ func (suite *ListsUnitSuite) TestConcatenateHyperlinkFields() { } } +func (suite *ListsUnitSuite) TestSetAdditionalDataByColumnNames() { + t := suite.T() + + tests := []struct { + name string + additionalData map[string]any + colName string + assertFn assert.BoolAssertionFunc + }{ + { + name: "choice column, single value", + additionalData: map[string]any{ + "choice": "good", + }, + colName: "choice", + assertFn: assert.False, + }, + { + name: "choice column, multiple values", + additionalData: map[string]any{ + "choice": []string{"good", "ok"}, + }, + colName: "choice", + assertFn: assert.True, + }, + { + name: "person column, single value", + additionalData: map[string]any{ + "PersonsLookupId": 10, + }, + colName: "PersonsLookupId", + assertFn: assert.False, + }, + { + name: "person column, multiple values", + additionalData: map[string]any{ + "Persons": []map[string]any{ + { + "LookupId": 10, + "LookupValue": "Who1", + "Email": "Who1@10rqc2.onmicrosoft.com", + }, + { + "LookupId": 11, + "LookupValue": "Who2", + "Email": "Who2@10rqc2.onmicrosoft.com", + }, + }, + }, + colName: "Persons", + assertFn: assert.True, + }, + } + + for _, test := range tests { + origFs := models.NewFieldValueSet() + origFs.SetAdditionalData(test.additionalData) + + colNames := map[string]*columnDetails{ + test.colName: {}, + } + + suite.Run(test.name, func() { + setAdditionalDataByColumnNames(origFs, colNames) + test.assertFn(t, colNames[test.colName].isMultipleEnabled) + }) + } +} + type ListsAPIIntgSuite struct { tester.Suite its intgTesterSetup From b770d0269d98319cfa9966a27bcb38a815a886d2 Mon Sep 17 00:00:00 2001 From: HiteshRepo Date: Wed, 24 Jan 2024 19:13:03 +0530 Subject: [PATCH 2/6] fix test TestColumnDefinitionable_LegacyColumns --- src/pkg/services/m365/api/lists_test.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/pkg/services/m365/api/lists_test.go b/src/pkg/services/m365/api/lists_test.go index 64dc0f1ecc..37cba33ac1 100644 --- a/src/pkg/services/m365/api/lists_test.go +++ b/src/pkg/services/m365/api/lists_test.go @@ -332,7 +332,7 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_LegacyColumns() { name string getList func() *models.List length int - expectedColNames map[string]any + expectedColNames map[string]*columnDetails }{ { name: "all legacy columns", @@ -346,7 +346,7 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_LegacyColumns() { return lst }, length: 0, - expectedColNames: map[string]any{TitleColumnName: nil}, + expectedColNames: map[string]*columnDetails{TitleColumnName: {}}, }, { name: "title and legacy columns", @@ -361,7 +361,7 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_LegacyColumns() { return lst }, length: 0, - expectedColNames: map[string]any{TitleColumnName: nil}, + expectedColNames: map[string]*columnDetails{TitleColumnName: {}}, }, { name: "readonly and legacy columns", @@ -376,7 +376,7 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_LegacyColumns() { return lst }, length: 0, - expectedColNames: map[string]any{TitleColumnName: nil}, + expectedColNames: map[string]*columnDetails{TitleColumnName: {}}, }, { name: "legacy and a text column", @@ -391,9 +391,9 @@ func (suite *ListsUnitSuite) TestColumnDefinitionable_LegacyColumns() { return lst }, length: 1, - expectedColNames: map[string]any{ - TitleColumnName: nil, - textColumnName: nil, + expectedColNames: map[string]*columnDetails{ + TitleColumnName: {}, + textColumnName: {}, }, }, } From 162d32efd479226ec6777f736bcb373f8af7f133 Mon Sep 17 00:00:00 2001 From: HiteshRepo Date: Thu, 25 Jan 2024 11:30:38 +0530 Subject: [PATCH 3/6] updates function name that sets multiple enabled by data type --- src/pkg/services/m365/api/lists.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/pkg/services/m365/api/lists.go b/src/pkg/services/m365/api/lists.go index cb9efe4ada..3cfb3ad7e6 100644 --- a/src/pkg/services/m365/api/lists.go +++ b/src/pkg/services/m365/api/lists.go @@ -487,7 +487,7 @@ func setAdditionalDataByColumnNames( for colName, colDetails := range columnNames { if val, ok := fieldData[colName]; ok { - setMultipleEnabled(val, colDetails) + setMultipleEnabledByFieldData(val, colDetails) filteredData[colName] = fieldData[colName] } @@ -498,12 +498,19 @@ func setAdditionalDataByColumnNames( return filteredData } -func setMultipleEnabled(val any, colDetails *columnDetails) { +func setMultipleEnabledByFieldData(val any, colDetails *columnDetails) { + // for columns like choice, even though it has an option to hold single/multiple values, + // the columnDefinition property 'allowMultipleValues' is not available. + // Hence we determine single/multiple from the actual field data. if reflect.TypeOf(val).Kind() == reflect.Slice { colDetails.isMultipleEnabled = true } } +// when creating list items with multiple values for a single column +// we let the API know that we are sending a collection. +// Hence this adds an additional field '@@odata.type' +// with value depending on type of column. func specifyODataType(filteredData map[string]any, colDetails *columnDetails, colName string) { if colDetails.isMultipleEnabled { filteredData[colName+ODataTypeFieldNamePart] = ODataTypeFieldNameStringVal From e2f2732fa88455eaea6b9a16e96f677b24e8b763 Mon Sep 17 00:00:00 2001 From: HiteshRepo Date: Thu, 25 Jan 2024 16:59:08 +0530 Subject: [PATCH 4/6] skips setting odatatype for text columns --- src/pkg/services/m365/api/lists.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/pkg/services/m365/api/lists.go b/src/pkg/services/m365/api/lists.go index 3cfb3ad7e6..4b297291a6 100644 --- a/src/pkg/services/m365/api/lists.go +++ b/src/pkg/services/m365/api/lists.go @@ -20,7 +20,8 @@ import ( var ErrSkippableListTemplate = clues.New("unable to create lists with skippable templates") type columnDetails struct { - isMultipleEnabled bool + isMultipleEnabled bool + hasDefaultedToText bool } // --------------------------------------------------------------------------- @@ -379,6 +380,8 @@ func setColumnType( orig models.ColumnDefinitionable, columnNames map[string]*columnDetails, ) { + colDetails := &columnDetails{} + switch { case orig.GetText() != nil: newColumn.SetText(orig.GetText()) @@ -409,10 +412,11 @@ func setColumnType( case orig.GetPersonOrGroup() != nil: newColumn.SetPersonOrGroup(orig.GetPersonOrGroup()) default: + colDetails.hasDefaultedToText = true newColumn.SetText(models.NewTextColumn()) } - columnNames[ptr.Val(newColumn.GetName())] = &columnDetails{} + columnNames[ptr.Val(newColumn.GetName())] = colDetails } // CloneListItem creates a new `SharePoint.ListItem` and stores the original item's @@ -512,6 +516,13 @@ func setMultipleEnabledByFieldData(val any, colDetails *columnDetails) { // Hence this adds an additional field '@@odata.type' // with value depending on type of column. func specifyODataType(filteredData map[string]any, colDetails *columnDetails, colName string) { + // text column itself does not allow holding multiple values + // some columns like 'term'/'managed metadata' have, + // but they get defaulted to text column. + if colDetails.hasDefaultedToText { + return + } + if colDetails.isMultipleEnabled { filteredData[colName+ODataTypeFieldNamePart] = ODataTypeFieldNameStringVal } From 98b7e5e6d839aa38b7cea3b3849d2fe8b048cbc7 Mon Sep 17 00:00:00 2001 From: HiteshRepo Date: Thu, 25 Jan 2024 17:10:32 +0530 Subject: [PATCH 5/6] fix lint --- src/pkg/services/m365/api/lists.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/pkg/services/m365/api/lists.go b/src/pkg/services/m365/api/lists.go index 4b297291a6..668a551e72 100644 --- a/src/pkg/services/m365/api/lists.go +++ b/src/pkg/services/m365/api/lists.go @@ -413,6 +413,7 @@ func setColumnType( newColumn.SetPersonOrGroup(orig.GetPersonOrGroup()) default: colDetails.hasDefaultedToText = true + newColumn.SetText(models.NewTextColumn()) } @@ -503,7 +504,7 @@ func setAdditionalDataByColumnNames( } func setMultipleEnabledByFieldData(val any, colDetails *columnDetails) { - // for columns like choice, even though it has an option to hold single/multiple values, + // for columns like 'choice', even though it has an option to hold single/multiple values, // the columnDefinition property 'allowMultipleValues' is not available. // Hence we determine single/multiple from the actual field data. if reflect.TypeOf(val).Kind() == reflect.Slice { From 55aed0de282f9a492c10374f4531af4287fd78a3 Mon Sep 17 00:00:00 2001 From: HiteshRepo Date: Tue, 30 Jan 2024 01:13:55 +0530 Subject: [PATCH 6/6] address review comment --- src/pkg/services/m365/api/lists.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/pkg/services/m365/api/lists.go b/src/pkg/services/m365/api/lists.go index 668a551e72..9dc26a783c 100644 --- a/src/pkg/services/m365/api/lists.go +++ b/src/pkg/services/m365/api/lists.go @@ -492,7 +492,12 @@ func setAdditionalDataByColumnNames( for colName, colDetails := range columnNames { if val, ok := fieldData[colName]; ok { - setMultipleEnabledByFieldData(val, colDetails) + // for columns like 'choice', even though it has an option to hold single/multiple values, + // the columnDefinition property 'allowMultipleValues' is not available. + // Hence we determine single/multiple from the actual field data. + if isSlice(val) { + colDetails.isMultipleEnabled = true + } filteredData[colName] = fieldData[colName] } @@ -503,15 +508,6 @@ func setAdditionalDataByColumnNames( return filteredData } -func setMultipleEnabledByFieldData(val any, colDetails *columnDetails) { - // for columns like 'choice', even though it has an option to hold single/multiple values, - // the columnDefinition property 'allowMultipleValues' is not available. - // Hence we determine single/multiple from the actual field data. - if reflect.TypeOf(val).Kind() == reflect.Slice { - colDetails.isMultipleEnabled = true - } -} - // when creating list items with multiple values for a single column // we let the API know that we are sending a collection. // Hence this adds an additional field '@@odata.type' @@ -679,3 +675,7 @@ func ListCollisionKey(list models.Listable) string { return ptr.Val(list.GetDisplayName()) } + +func isSlice(val any) bool { + return reflect.TypeOf(val).Kind() == reflect.Slice +}