Skip to content

Define APIs for scoped connector definitions #11244

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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Mar 22, 2022
Merged

Define APIs for scoped connector definitions #11244

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
f7433f2
listActorDefinitions apis now require workspaceId
git-phu Mar 16, 2022
ff61ece
actorDefinitionCreate apis now require workspaceId
git-phu Mar 17, 2022
84b35ed
source_definitions/list_opt_in
git-phu Mar 17, 2022
1acbc47
destination_definitions/list_opt_in
git-phu Mar 17, 2022
9431fcb
create/delete source definition grants
git-phu Mar 17, 2022
4c97d37
create/delete destination definition grants
git-phu Mar 17, 2022
3df0873
PR feedback
git-phu Mar 18, 2022
0bcc6e0
new create/list source definition routes
git-phu Mar 21, 2022
d6cbc19
new create/list destination definition routes
git-phu Mar 21, 2022
f72e21c
revert acceptance test changes
git-phu Mar 21, 2022
00a9186
new update/delete actor definition routes
git-phu Mar 21, 2022
c902119
rearrange routes in config.yaml
git-phu Mar 21, 2022
8192aff
PR feedback
git-phu Mar 21, 2022
3e11a1a
new create actor definition routes
git-phu Mar 22, 2022
3a3b4cd
typo
git-phu Mar 22, 2022
d362ff8
typo
git-phu Mar 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
194 changes: 194 additions & 0 deletions airbyte-api/src/main/openapi/config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,11 @@ paths:
- source_definition
summary: List all the sourceDefinitions the current Airbyte deployment is configured to use
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should probably be tweaked slightly, maybe 'List all the sourceDefinitions the indicated workspace is configured to use'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we decided not to modify the params of the existing endpoints, when implementing the new endpoint behaviors I will also update the summaries of the existing actor definition endpoints to reflect their new behavior.

operationId: listSourceDefinitions
requestBody:
content:
application/json:
schema:
$ref: "#/components/schemas/WorkspaceIdRequestBody"
Copy link
Contributor

Choose a reason for hiding this comment

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

I do wonder if it would make sense to support a listSourceDefinitions route that doesn't require a workspaceId in the request body. Semantically, such a route would mean 'list all the sourceDefinitions that are available by default to the current Airbyte deployment' whereas your modified route semantically means 'list all the sourceDefinitions that are available to the indicated workspace within the current Airbyte deployment'.

If we don't have a use-case for the workspace-agnostic version of the route, maybe we don't worry about it. @cgardens what do you think? Are there situations where we think we'd want to list all of the default, public definitions for the current Airbyte instance without requiring a workspace ID?

Copy link
Contributor

Choose a reason for hiding this comment

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

good call. i think it's valuable to have that functionality for us as administrators of cloud. so yeah i think i would keep both, but change the permission so it was only usable by instance admins.

also worth noting that this might make our lives easier for the sake of migration. right now if we merge this PR, the app will break because the UI isn't ready for it. ideally we could merge this PR without having to coordinate with the UI team. i.e.

  • make change in API that is backwards compatible
  • tell the UI team that the new endpoint exists
  • once the UI team has updated the UI, we adjust the old endpoint to have the behavior i mentioned (instance-wide view only for instance admins)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of having this PR only make non-breaking changes.

And similarly to the list apis, I think we could also add new endpoints for creating custom definitions for a specific workspace and leave the existing create endpoints for admins only to create private non-custom definitions (i.e. the definitions that are grantable to workspaces).

Also a slight tangent, but my initial thought for defining these new endpoints with more restful routes would be something like

/v1/workspaces/{workspace_id}/source_definitions/list
/v1/workspaces/{workspace_id}/source_definitions/create
/v1/workspaces/{workspace_id}/source_definitions/{source_definitions}/grant
/v1/workspaces/{workspace_id}/source_definitions/{source_definitions}/revoke

but it looks like we don't actually use restful routes in this app, which is fine but I'm just curious about how we decided on the naming convention we use for routes?

Copy link
Contributor

Choose a reason for hiding this comment

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

The description at the top explains WHAT the convention is a bit. In terms of the WHY... it was inspired by how slack does it. If I were to start again, I would just go with REST because the fact that this approach is unfamiliar to most engineers is enough to make it not really worth it. It does iron over some parts of REST that are annoying and I liked the idea of modeling things in a more RPC style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, I do actually kinda like the current approach and I can see how it's convenient to always have all args come from the post body too. Just takes me a bit more effort to come up with sensible names for the routes 😄

responses:
"200":
description: Successful operation
Expand All @@ -317,6 +322,66 @@ paths:
application/json:
schema:
$ref: "#/components/schemas/SourceDefinitionReadList"
/v1/source_definitions/list_opt_in:
post:
tags:
- source_definition
summary: List all opt-in sourceDefinitions along with the current workspace's opt-in statuses
operationId: listSourceDefinitionOptIns
requestBody:
content:
application/json:
schema:
$ref: "#/components/schemas/WorkspaceIdRequestBody"
responses:
"200":
description: Successful operation
content:
application/json:
schema:
$ref: "#/components/schemas/SourceDefinitionOptInReadList"
/v1/source_definitions/grant_definition:
post:
tags:
- source_definition
summary: Opt in to a non-public, non-custom sourceDefinition
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unsure on whether or not we actually want to specify 'non-custom' for this route. I could see a scenario where, for instance, an admin logs into somebody's workspace and adds a custom connector definition, verifies that it works, and then wants to add the same custom connector to another workspace. In that case, we might want it to be possible for the instance admin to hit this API endpoint to grant access to the 2nd workspace, instead of forcing them to add it as a duplicate custom connector, or needing to add it to the catalog as a public: false "opt-in" definition.

What do you think? @cgardens also curious about your opinion on this. I think there's potentially some risk in allowing instance admins to create grants for custom connectors that aren't in our catalog, but maybe it's worth it if there's enough utility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah i think either could work. I believe @cgardens had mentioned a scenario to me before that we'd want to avoid, which is: we don't want a custom connector that is specifically built for one workspace (possibly even with workspace specific secrets baked into the connector definition) to accidentally appear in other workspaces.

Maybe if we included some indicator of which private connectors were custom or not, that could help alleviate this concern.

Copy link
Contributor

Choose a reason for hiding this comment

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

interesting point. i think since we don't have a notion of an "owner" for a connector definition, what peter said about not allowing this for custom connectors makes sense. otherwise if someone got ahold of the definition id for a custom connector that wasn't theirs they could add it to their own workspace. so either for custom connectors we need to introduce some concept of owners (it makes my head hurt to think about how we would actually model it) or we don't allow granting access to custom connectors in the api. i think i'm inclined to stick with the ladder for now and we can build more features into the schema in the future if we need it. lmk if i'm overestimating the definition ownership piece.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, that makes sense to me! I think we're good to leave this as-is then

operationId: createSourceDefinitionOptIn
requestBody:
content:
application/json:
schema:
$ref: "#/components/schemas/SourceDefinitionOptInUpdate"
required: true
responses:
"200":
description: Successful operation
content:
application/json:
schema:
$ref: "#/components/schemas/SourceDefinitionOptInRead"
"404":
$ref: "#/components/responses/NotFoundResponse"
"422":
$ref: "#/components/responses/InvalidInputResponse"
/v1/source_definitions/revoke_definition:
post:
tags:
- source_definition
summary: Opt out of a non-public, non-custom sourceDefinition
operationId: deleteSourceDefinitionOptIn
requestBody:
content:
application/json:
schema:
$ref: "#/components/schemas/SourceDefinitionOptInUpdate"
required: true
responses:
"204":
description: The resource was deleted successfully.
"404":
$ref: "#/components/responses/NotFoundResponse"
"422":
$ref: "#/components/responses/InvalidInputResponse"
/v1/source_definitions/get:
post:
tags:
Expand Down Expand Up @@ -654,6 +719,11 @@ paths:
- destination_definition
summary: List all the destinationDefinitions the current Airbyte deployment is configured to use
operationId: listDestinationDefinitions
requestBody:
content:
application/json:
schema:
$ref: "#/components/schemas/WorkspaceIdRequestBody"
responses:
"200":
description: Successful operation
Expand All @@ -675,6 +745,66 @@ paths:
application/json:
schema:
$ref: "#/components/schemas/DestinationDefinitionReadList"
/v1/destination_definitions/list_opt_in:
post:
tags:
- destination_definition
summary: List all opt-in destinationDefinitions along with the current workspace's opt-in statuses
operationId: listDestinationDefinitionOptIns
requestBody:
content:
application/json:
schema:
$ref: "#/components/schemas/WorkspaceIdRequestBody"
responses:
"200":
description: Successful operation
content:
application/json:
schema:
$ref: "#/components/schemas/DestinationDefinitionOptInReadList"
/v1/destination_definitions/grant_definition:
post:
tags:
- destination_definition
summary: Opt in to a non-public, non-custom destinationDefinition
operationId: createDestinationDefinitionOptIn
requestBody:
content:
application/json:
schema:
$ref: "#/components/schemas/DestinationDefinitionOptInUpdate"
required: true
responses:
"200":
description: Successful operation
content:
application/json:
schema:
$ref: "#/components/schemas/DestinationDefinitionOptInRead"
"404":
$ref: "#/components/responses/NotFoundResponse"
"422":
$ref: "#/components/responses/InvalidInputResponse"
/v1/destination_definitions/revoke_definition:
post:
tags:
- destination_definition
summary: Opt out of a non-public, non-custom destinationDefinition
operationId: deleteDestinationDefinitionOptIn
requestBody:
content:
application/json:
schema:
$ref: "#/components/schemas/DestinationDefinitionOptInUpdate"
required: true
responses:
"204":
description: The resource was deleted successfully.
"404":
$ref: "#/components/responses/NotFoundResponse"
"422":
$ref: "#/components/responses/InvalidInputResponse"
/v1/destination_definitions/get:
post:
tags:
Expand Down Expand Up @@ -2108,11 +2238,14 @@ components:
SourceDefinitionCreate:
type: object
required:
- workspaceId
- name
- dockerRepository
- dockerImageTag
- documentationUrl
properties:
workspaceId:
$ref: "#/components/schemas/WorkspaceId"
name:
type: string
dockerRepository:
Expand Down Expand Up @@ -2177,6 +2310,35 @@ components:
type: array
items:
$ref: "#/components/schemas/SourceDefinitionRead"
SourceDefinitionOptInRead:
type: object
required:
- sourceDefinition
- optIn
properties:
sourceDefinition:
$ref: "#/components/schemas/SourceDefinitionRead"
optIn:
type: boolean
SourceDefinitionOptInReadList:
type: object
required:
- sourceDefinitionOptIns
properties:
sourceDefinitionOptIns:
type: array
items:
$ref: "#/components/schemas/SourceDefinitionOptInRead"
SourceDefinitionOptInUpdate:
type: object
required:
- sourceDefinitionId
- workspaceId
properties:
sourceDefinitionId:
$ref: "#/components/schemas/SourceDefinitionId"
workspaceId:
$ref: "#/components/schemas/WorkspaceId"
# SOURCE SPECIFICATION
SourceDefinitionSpecification:
description: The specification for what values are required to configure the sourceDefinition.
Expand Down Expand Up @@ -2392,11 +2554,14 @@ components:
DestinationDefinitionCreate:
type: object
required:
- workspaceId
- name
- dockerRepository
- dockerImageTag
- documentationUrl
properties:
workspaceId:
$ref: "#/components/schemas/WorkspaceId"
name:
type: string
dockerRepository:
Expand Down Expand Up @@ -2461,6 +2626,35 @@ components:
type: array
items:
$ref: "#/components/schemas/DestinationDefinitionRead"
DestinationDefinitionOptInRead:
type: object
required:
- destinationDefinition
- optIn
properties:
destinationDefinition:
$ref: "#/components/schemas/DestinationDefinitionRead"
optIn:
type: boolean
DestinationDefinitionOptInReadList:
type: object
required:
- destinationDefinitionOptIns
properties:
destinationDefinitionOptIns:
type: array
items:
$ref: "#/components/schemas/DestinationDefinitionOptInRead"
DestinationDefinitionOptInUpdate:
type: object
required:
- destinationDefinitionId
- workspaceId
properties:
destinationDefinitionId:
$ref: "#/components/schemas/DestinationDefinitionId"
workspaceId:
$ref: "#/components/schemas/WorkspaceId"
# DESTINATION DEFINITION SPECIFICATION
DestinationDefinitionSpecification:
description: The specification for what values are required to configure the destinationDefinition.
Expand Down
40 changes: 38 additions & 2 deletions airbyte-server/src/main/java/io/airbyte/server/apis/ConfigurationApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
import io.airbyte.api.model.DestinationCreate;
import io.airbyte.api.model.DestinationDefinitionCreate;
import io.airbyte.api.model.DestinationDefinitionIdRequestBody;
import io.airbyte.api.model.DestinationDefinitionOptInRead;
import io.airbyte.api.model.DestinationDefinitionOptInReadList;
import io.airbyte.api.model.DestinationDefinitionOptInUpdate;
import io.airbyte.api.model.DestinationDefinitionRead;
import io.airbyte.api.model.DestinationDefinitionReadList;
import io.airbyte.api.model.DestinationDefinitionSpecificationRead;
Expand Down Expand Up @@ -58,6 +61,9 @@
import io.airbyte.api.model.SourceCreate;
import io.airbyte.api.model.SourceDefinitionCreate;
import io.airbyte.api.model.SourceDefinitionIdRequestBody;
import io.airbyte.api.model.SourceDefinitionOptInRead;
import io.airbyte.api.model.SourceDefinitionOptInReadList;
import io.airbyte.api.model.SourceDefinitionOptInUpdate;
import io.airbyte.api.model.SourceDefinitionRead;
import io.airbyte.api.model.SourceDefinitionReadList;
import io.airbyte.api.model.SourceDefinitionSpecificationRead;
Expand Down Expand Up @@ -305,7 +311,7 @@ public NotificationRead tryNotificationConfig(final Notification notification) {
// SOURCE

@Override
public SourceDefinitionReadList listSourceDefinitions() {
public SourceDefinitionReadList listSourceDefinitions(final WorkspaceIdRequestBody workspaceIdRequestBody) {
return execute(sourceDefinitionsHandler::listSourceDefinitions);
}

Expand All @@ -314,6 +320,11 @@ public SourceDefinitionReadList listLatestSourceDefinitions() {
return execute(sourceDefinitionsHandler::listLatestSourceDefinitions);
}

@Override
public SourceDefinitionOptInReadList listSourceDefinitionOptIns(final WorkspaceIdRequestBody workspaceIdRequestBody) {
return null;
}

@Override
public SourceDefinitionRead getSourceDefinition(final SourceDefinitionIdRequestBody sourceDefinitionIdRequestBody) {
return execute(() -> sourceDefinitionsHandler.getSourceDefinition(sourceDefinitionIdRequestBody));
Expand All @@ -324,6 +335,11 @@ public SourceDefinitionRead createSourceDefinition(final SourceDefinitionCreate
return execute(() -> sourceDefinitionsHandler.createCustomSourceDefinition(sourceDefinitionCreate));
}

@Override
public SourceDefinitionOptInRead createSourceDefinitionOptIn(final SourceDefinitionOptInUpdate sourceDefinitionOptInUpdate) {
return null;
}

@Override
public SourceDefinitionRead updateSourceDefinition(final SourceDefinitionUpdate sourceDefinitionUpdate) {
return execute(() -> sourceDefinitionsHandler.updateSourceDefinition(sourceDefinitionUpdate));
Expand All @@ -337,6 +353,11 @@ public void deleteSourceDefinition(final SourceDefinitionIdRequestBody sourceDef
});
}

@Override
public void deleteSourceDefinitionOptIn(final SourceDefinitionOptInUpdate sourceDefinitionOptInUpdate) {

}

// SOURCE SPECIFICATION

@Override
Expand Down Expand Up @@ -452,7 +473,7 @@ public DbMigrationExecutionRead executeMigrations(final DbMigrationRequestBody r
// DESTINATION

@Override
public DestinationDefinitionReadList listDestinationDefinitions() {
public DestinationDefinitionReadList listDestinationDefinitions(final WorkspaceIdRequestBody workspaceIdRequestBody) {
return execute(destinationDefinitionsHandler::listDestinationDefinitions);
}

Expand All @@ -461,6 +482,11 @@ public DestinationDefinitionReadList listLatestDestinationDefinitions() {
return execute(destinationDefinitionsHandler::listLatestDestinationDefinitions);
}

@Override
public DestinationDefinitionOptInReadList listDestinationDefinitionOptIns(final WorkspaceIdRequestBody workspaceIdRequestBody) {
return null;
}

@Override
public DestinationDefinitionRead getDestinationDefinition(final DestinationDefinitionIdRequestBody destinationDefinitionIdRequestBody) {
return execute(() -> destinationDefinitionsHandler.getDestinationDefinition(destinationDefinitionIdRequestBody));
Expand All @@ -471,6 +497,11 @@ public DestinationDefinitionRead createDestinationDefinition(final DestinationDe
return execute(() -> destinationDefinitionsHandler.createCustomDestinationDefinition(destinationDefinitionCreate));
}

@Override
public DestinationDefinitionOptInRead createDestinationDefinitionOptIn(final DestinationDefinitionOptInUpdate destinationDefinitionOptInUpdate) {
return null;
}

@Override
public DestinationDefinitionRead updateDestinationDefinition(final DestinationDefinitionUpdate destinationDefinitionUpdate) {
return execute(() -> destinationDefinitionsHandler.updateDestinationDefinition(destinationDefinitionUpdate));
Expand All @@ -484,6 +515,11 @@ public void deleteDestinationDefinition(final DestinationDefinitionIdRequestBody
});
}

@Override
public void deleteDestinationDefinitionOptIn(final DestinationDefinitionOptInUpdate destinationDefinitionOptInUpdate) {

}

// DESTINATION SPECIFICATION

@Override
Expand Down
Loading