Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(api): [NAN-793] bulk metadata update api #2145

Merged
merged 34 commits into from May 20, 2024

Conversation

khaliqgant
Copy link
Member

@khaliqgant khaliqgant commented May 13, 2024

Describe your changes

Update the set and update metadata endpoints to accept an array of connection ids. If any invalid connection is passed in at any point in the array the entire operation is bailed and no metadata is set.

Issue ticket number and link

NAN-793

Checklist before requesting a review (skip if just adding/editing APIs & templates)

  • I added tests, otherwise the reason is:
  • I added observability, otherwise the reason is:
  • I added analytics, otherwise the reason is:

Copy link

linear bot commented May 13, 2024

@@ -522,7 +522,9 @@
"reference/api/connection/get",
"reference/api/connection/post",
"reference/api/connection/set-metadata",
"reference/api/connection/bulk-set-metadata",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need new endpoints for this? Can we just support both a single value and an array in the existing routes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That what I would have liked to do but we can't without deprecating this current method. This is because the connectionId is passed as part of the URL:

curl --location --request PATCH 'http://localhost:3003/connection/to-override/metadata?provider_config_key=google-calendar' \
--header 'Content-Type: application/json' \
--data '{
    "myMetadata": "bar"
}'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. In this case, I would aim to make this endpoint the new standard, and later deprecate the other one, so we keep a minimal number of endpoints.

In order to do so, I would remove bulk from the endpoint and have something like:
PATCH https://api.nango.dev/connection/update-metadata
POST https://api.nango.dev/connection/set-metadata

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's logical. It is mirroring the endpoints to modify metadata for a single connection

// single connection
POST /connection/:connectionId/metadata
PATCH /connection/:connectionId/metadata

// multiple connection
POST /connection/metadata
PATCH /connection/metadata

docs-v2/reference/sdks/node.mdx Outdated Show resolved Hide resolved
docs-v2/reference/sdks/node.mdx Show resolved Hide resolved
docs-v2/reference/sdks/node.mdx Outdated Show resolved Hide resolved
docs-v2/reference/sdks/node.mdx Outdated Show resolved Hide resolved
packages/server/lib/controllers/connection.controller.ts Outdated Show resolved Hide resolved
packages/server/lib/controllers/connection.controller.ts Outdated Show resolved Hide resolved
docs-v2/spec.yaml Outdated Show resolved Hide resolved
docs-v2/spec.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@bodinsamuel bodinsamuel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌🏻
It would have been great to use the new pattern of creating routes, just to have some payload validation and test but not blocking

@khaliqgant
Copy link
Member Author

# POST: set metadata
curl --location '/connection/metadata?provider_config_key=unauthenticated' \
--data '{
    "metadata": {
        "my_metaddata": "no"
    },
    "connection_id": ["one", "two-connections"]
}'

# PATCH update metadata
curl --location --request PATCH '/connection/metadata?provider_config_key=unauthenticated' \
--data '{
    "metadata": {
        "updated-metadata": "meta"
    },
    "connection_id": ["one"]
}'

The older metadata endpoints still exist but now exist in the documentation marked as legacy.

One more pass is requested to the interface and implementation @bastienbeurier @TBonnin @bodinsamuel

@khaliqgant khaliqgant requested a review from TBonnin May 15, 2024 18:24
docs-v2/spec.yaml Outdated Show resolved Hide resolved
docs-v2/spec.yaml Show resolved Hide resolved
patch:
description: Edit custom metadata for the connection or connections (only overrides specified properties, not the entire metadata). If updating multiple connections pass in an array of connection ids instead of a string.
parameters:
- name: provider_config_key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here about the query apram

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

docs-v2/reference/sdks/node.mdx Show resolved Hide resolved
docs-v2/reference/sdks/node.mdx Show resolved Hide resolved
docs-v2/reference/sdks/node.mdx Show resolved Hide resolved
packages/server/lib/controllers/connection.controller.ts Outdated Show resolved Hide resolved
@khaliqgant
Copy link
Member Author

👌🏻 It would have been great to use the new pattern of creating routes, just to have some payload validation and test but not blocking

Added with 81943a6

public async updateMetadata(connections: Connection[], metadata: Metadata): Promise<void> {
for (const connection of connections) {
const newMetadata = { ...connection.metadata, ...metadata };
await this.replaceMetadata([connection.id as number], newMetadata);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would have been nice to have all the queries as part of a transaction. Here we might potentially have some out of sync Metadata if one or more query fails somehow

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with 25669dc

@khaliqgant khaliqgant merged commit 960c0e8 into master May 20, 2024
19 checks passed
@khaliqgant khaliqgant deleted the khaliq/nan-793-bulk-metadata-update-api-planning branch May 20, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants