feat: Deep connector MS Dynamics365 Sales (Read) #157

Cobalt0s · 2024-04-03T13:11:28Z

Closes #186

Cobalt0s

@RajatPawar, here is an initial explanation and questions to existing/future code changes

Cobalt0s · 2024-04-04T19:59:35Z

common/error.go

@@ -31,3 +31,14 @@ func InterpretError(res *http.Response, body []byte) error {

 	return NewHTTPStatusError(res.StatusCode, fmt.Errorf("%w: %s", ErrUnknown, string(body)))
 }
+
+func PanicRecovery(wrapup func(cause error)) {


Every connector captures panic errors and returns it as normal error. Wrapup is a callback after recovery

Cobalt0s · 2024-04-04T20:02:33Z

common/interpreter/error.go

+	}
+
+	if h.JSON != nil && mediaType == "application/json" {
+		return h.JSON(res, body)


Depending on a ContentType a connector can parse error response in JSON, XML, HTML whatever impl we want to provide.
Every connector understands JSON structure in their own way. Other media types could be added.

Cobalt0s · 2024-04-04T20:04:17Z

common/json.go

+	// empty response body should not be parsed as JSON since it will cause ajson to err
+	if len(body) == 0 {
+		return nil, nil //nolint:nilnil
+	}


it looks like every CRUD operation does this check (except GET, could be a bug), pulled it to this method since all of them call it at the end.

Cobalt0s · 2024-04-04T20:08:47Z

common/paramsbuilder/params.go

+// ParamAssurance checks that param data is valid
+// Every param instance must implement it.
+type ParamAssurance interface {
+	ValidateParams() error


There are several parameters with single responsibilities: Client/Workspace/Module. There could be more of these shared parameters.
Every parameter must determine if its value is required/optional by implementing ParamAssurance.

This is a very cool package, thanks for introducing it!

Cobalt0s · 2024-04-04T20:11:31Z

common/parse.go

+
+	// JSONManager is a helpful wrapper of ajson library that adds errors when querying JSON payload
+	// and provides common conversion methods.
+	JSONManager = jsonManager{}


Querying JSON content deals with finding "keys" and validating expected data type of the "value".
ErrNotArray, ErrNotObject errors can be universally shared for all JSON parsers

great idea!

Cobalt0s · 2024-04-04T20:23:00Z

msdsales/params.go

+		opt(params)
+	}
+
+	return params, params.ValidateParams()


This is a construcotr of connector parameters from Option functions with required field validation

Cobalt0s · 2024-04-04T20:26:09Z

msdsales/parse.go

+// * singular record
+// * list of records
+// * hybrid, list of records with extra fields describing list.
+func getMarshaledData(records []map[string]interface{}, fields []string) ([]common.ReadResultRow, error) {


Can we marshal singular record in case of
GET domain/cats/<catID> vs GET domain/cats
Also LIST of cats could return additional parameters stored outside of array, How those fields are returned?

For now, our connector only does LIST and that's ok. We don't need to support the GET use case yet.

I will remove the FIXME comment. I guess for future I was wondering if fields could perform queries like: [*].name, [*].meta.createdAt, listType, expectedNumRecords.

Cobalt0s · 2024-04-04T20:28:52Z

msdsales/read.go

+// $filter = query functions, comparisons
+// TODO pagination
+// $top = <int> of entries to return (ignored if header <Prefer: odata.maxpagesize>)
+// $count = counts all existing rows (@odata.count)


Microsoft's Read is very flexible, see options. Should we support all query capabilities?

https://www.loom.com/share/5b109408daf9416aa07ac9427e13e228

Thanks for pointing this out @Cobalt0s! For now, we will just support the basic functionality of querying by field names, but this is really great to know. Can you replace the comments on line 13 to 24 with something like:

// Microsoft API supports other capabilities like filtering, grouping, and sorting which we can potentially tap into later. // See https://learn.microsoft.com/en-us/power-apps/developer/data-platform/webapi/query-data-web-api#odata-query-options

Yes, bottom line is I have described these options in docs.

msdsales/read.go

Cobalt0s · 2024-04-04T20:33:24Z

test/msdsales/paginate/main.go

+	}
+
+	res, err = conn.Read(ctx, common.ReadParams{
+		NextPage: string(res.NextPage), // TODO should Read params have NextPageToken type?


ReadParams.NextPage(string) <- doesn't match type with -> ReadResult.NextPage(NextPageToken)

Great catch, can you update the ReadParams struct in common/types.go to have NextPage be of the type NextPageToken token?

Changed.
In places where it was expected to use as string instead of direct conversion I added a method. Let me know if it is not ok.

type NextPageToken string func (t NextPageToken) String() string { return string(t) }

Cobalt0s · 2024-04-08T12:34:50Z

test/msdsales/read/main.go

+	"github.com/amp-labs/connectors/test/utils"
+)
+
+func main() {


https://www.loom.com/share/36a18cac5a09411680fe864ee585da60

Cobalt0s · 2024-04-08T14:26:32Z

common/reqrepeater/standard.go

+// Strategy is configuration of how a connector handles faulty requests
+// connector will create Retry based on this config every time it attempts to make API call.
+type Strategy interface {
+	Start() Retry


https://www.loom.com/share/39f1d4ea8dc446148eeff83a3f1c8681

Cobalt0s · 2024-04-08T14:33:08Z

msdsales/params.go

+type parameters struct {
+	paramsbuilder.Client
+	paramsbuilder.Workspace
+	paramsbuilder.Module


https://www.loom.com/share/97b52ea5eeab449cb0dd00abc2842785

laurenzlong · 2024-04-15T21:33:10Z

common/parse.go

+
+	// JSONManager is a helpful wrapper of ajson library that adds errors when querying JSON payload
+	// and provides common conversion methods.
+	JSONManager = jsonManager{}


great idea!

laurenzlong · 2024-04-15T22:16:33Z

common/reqrepeater/standard.go

+// Strategy is configuration of how a connector handles faulty requests
+// connector will create Retry based on this config every time it attempts to make API call.
+type Strategy interface {
+	Start() Retry


Hey @Cobalt0s while this is a really good idea, we actually already handle retries in the server. We're almost done with building a sophisticated rate limiter that handles retries and employs different retry intervals according to the provider and other overrides that can be builder or consumer configurable. The overall design principle we're using is that the Connectors library handles making API calls & result + error parsing, while orchestration is handled by the server. So can you please remove the reqrepeater package?

msdsales/read_test.go

laurenzlong · 2024-04-15T22:32:54Z

msdsales/read_test.go

+					"garbage": {}
+				}`)
+			})),
+			expectedErrs: []error{errors.New("wrong request: wrong key 'value'")},


Hard coding a particular error string to expect leads to a fragile test case because if we modified our error message later, this test will break. The test case right above is OK since the mock server returns a specific error string, so it's ok to hardcode that in expectedErrs

Yes, this is a smell code. There was another test where I used type comparison. To be more general I added reflection check. For this test checking that error comes from ajson lib should be enough.

expectedErrs - used when you know exact error expectedErrTypes - used less frequently and relies on type rather than content

msdsales/read_test.go

laurenzlong · 2024-04-15T23:43:48Z

test/msdsales/paginate/main.go

+	}
+
+	res, err = conn.Read(ctx, common.ReadParams{
+		NextPage: string(res.NextPage), // TODO should Read params have NextPageToken type?


Great catch, can you update the ReadParams struct in common/types.go to have NextPage be of the type NextPageToken token?

test/msdsales/paginate/main.go

laurenzlong · 2024-04-16T00:08:29Z

common/paramsbuilder/params.go

+// ParamAssurance checks that param data is valid
+// Every param instance must implement it.
+type ParamAssurance interface {
+	ValidateParams() error


This is a very cool package, thanks for introducing it!

msdsales/errors_test.go

msdsales/methods.go

laurenzlong

Thanks so much!

RajatPawar self-requested a review April 4, 2024 07:25

Cobalt0s force-pushed the deep_ms_sales branch 4 times, most recently from a1de53f to 779f196 Compare April 4, 2024 19:57

Cobalt0s commented Apr 4, 2024

View reviewed changes

RajatPawar requested review from eberle1080 and laurenzlong April 5, 2024 09:44

laurenzlong requested a review from jlimatampersand April 5, 2024 18:34

Cobalt0s force-pushed the deep_ms_sales branch from 1381b97 to 05d6516 Compare April 8, 2024 10:55

Cobalt0s commented Apr 8, 2024

View reviewed changes

Cobalt0s marked this pull request as ready for review April 8, 2024 14:38

Cobalt0s mentioned this pull request Apr 8, 2024

feat: Deep connector MS Dynamics365 CRM (ListMetadata) #179

Merged

Cobalt0s changed the title ~~feat: Deep connector MS Dynamics365 Sales~~ feat: Deep connector MS Dynamics365 Sales (Read) Apr 9, 2024

Cobalt0s force-pushed the deep_ms_sales branch from 05d6516 to ab4aad7 Compare April 9, 2024 14:50

Cobalt0s mentioned this pull request Apr 10, 2024

feat: Deep connector MS Dynamics365 Sales (Write) #202

Merged

RajatPawar requested review from eberle1080, laurenzlong, RajatPawar and jlimatampersand and removed request for eberle1080, laurenzlong, RajatPawar and jlimatampersand April 10, 2024 17:19

laurenzlong reviewed Apr 16, 2024

View reviewed changes

Cobalt0s force-pushed the deep_ms_sales branch from ab4aad7 to 1a007e5 Compare April 16, 2024 11:49

Cobalt0s added 3 commits April 16, 2024 14:59

feat: Deep connector MS Dynamics365 Sales (Read)

7117181

address comments

1cd8ee1

lint

e847df6

Cobalt0s force-pushed the deep_ms_sales branch from 1a007e5 to e847df6 Compare April 16, 2024 13:02

laurenzlong approved these changes Apr 16, 2024

View reviewed changes

laurenzlong merged commit 0e98118 into amp-labs:main Apr 16, 2024
4 checks passed

Cobalt0s deleted the deep_ms_sales branch April 16, 2024 19:15

Cobalt0s self-assigned this May 21, 2024

Cobalt0s added the read-connector label May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Deep connector MS Dynamics365 Sales (Read) #157

feat: Deep connector MS Dynamics365 Sales (Read) #157

Cobalt0s commented Apr 3, 2024 •

edited

Loading

Cobalt0s left a comment

Cobalt0s Apr 4, 2024

Cobalt0s Apr 4, 2024

Cobalt0s Apr 4, 2024

Cobalt0s Apr 4, 2024

laurenzlong Apr 16, 2024

Cobalt0s Apr 4, 2024

laurenzlong Apr 15, 2024

Cobalt0s Apr 4, 2024

Cobalt0s Apr 4, 2024

laurenzlong Apr 15, 2024

Cobalt0s Apr 16, 2024

Cobalt0s Apr 4, 2024

Cobalt0s Apr 10, 2024

laurenzlong Apr 15, 2024

Cobalt0s Apr 16, 2024

Cobalt0s Apr 4, 2024

laurenzlong Apr 15, 2024

Cobalt0s Apr 16, 2024

Cobalt0s Apr 8, 2024

Cobalt0s Apr 8, 2024

Cobalt0s Apr 8, 2024

laurenzlong Apr 15, 2024

laurenzlong Apr 15, 2024

laurenzlong Apr 15, 2024

Cobalt0s Apr 16, 2024

laurenzlong Apr 15, 2024

laurenzlong Apr 16, 2024

laurenzlong left a comment •

edited

Loading

feat: Deep connector MS Dynamics365 Sales (Read) #157

feat: Deep connector MS Dynamics365 Sales (Read) #157

Conversation

Cobalt0s commented Apr 3, 2024 • edited Loading

Cobalt0s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurenzlong left a comment • edited Loading

Choose a reason for hiding this comment

Cobalt0s commented Apr 3, 2024 •

edited

Loading

laurenzlong left a comment •

edited

Loading