Refactor Catalog API #1934
Refactor Catalog API #1934
Conversation
ChristopheDuong
force-pushed
the
chris/api_catalog
branch
from
5d10a77 to
26fe2da
Compare
airbyte-server/src/main/java/io/airbyte/server/converters/SchemaConverter.java
Outdated
Show resolved
Hide resolved
| io.airbyte.api.model.AirbyteStreamConfiguration result = new AirbyteStreamConfiguration() | ||
| .cleanedName(Names.toAlphanumericAndUnderscore(s.getName())) | ||
| .cursorField(s.getDefaultCursorField()) | ||
| .selected(true); |
There was a problem hiding this comment.
i don't like that these converters still aren't symmetrical. this is a fault in the BE data model (not in your implementation). we still lose information as the configuration comes back from the API. can you create an issue to make this symmetrical? i think ideally the backend model should look more like the FE. keeping all of the streams in the list but having a flag as to whether they are going to be synced. it's a bigger change though and we should not try to do it in this PR, but want to make sure we follow up.
airbyte-server/src/main/java/io/airbyte/server/converters/SchemaConverter.java
Outdated
Show resolved
Hide resolved
airbyte-server/src/main/java/io/airbyte/server/handlers/ConnectionsHandler.java
Outdated
Show resolved
Hide resolved
airbyte-server/src/test/java/io/airbyte/server/converters/SchemaConverterTest.java
Outdated
Show resolved
Hide resolved
|
|
||
| final StandardSync standardSync = new StandardSync() | ||
| .withConnectionId(connectionId) | ||
| .withDestinationId(destinationId) | ||
| .withSourceId(sourceId) | ||
| .withStatus(StandardSync.Status.ACTIVE) | ||
| .withName(CONNECTION_NAME) | ||
| .withSchema(schema); | ||
| .withSchema(catalog); |
There was a problem hiding this comment.
this should probably be called catalog instead of schema. i'm sympathetic that changing these can be a bit of a pain, so if this is really awful to fix don't worry about it. but since we're tearing everythhing up now, this is as good a time as any.
There was a problem hiding this comment.
Don't worry, it's not as painful as changing completely the object underneath haha
(at least on my side, I don't know if this affects @jamakase too much or not)
There was a problem hiding this comment.
I don't think it should affect him since this model is only used behind the API. the only thing that should affect him are changes to the API interface (in config.yaml).
| $ref: "#/components/schemas/SourceSchemaStream" | ||
| SourceSchemaStream: | ||
| $ref: "#/components/schemas/AirbyteStreamAndConfiguration" | ||
| AirbyteStreamAndConfiguration: |
There was a problem hiding this comment.
What's the point of prepending entities with Airbyte? Most of the other fields don't have this prefix. What is more, I do not think that it actually brings any meaning to prepend all fields with Airbyte
There was a problem hiding this comment.
Also, @ChristopheDuong do you think its a good idea to combine different parts of this object with And? What if we add 1 more field here somewhere in future? It means that the current name will be obsolete.
There was a problem hiding this comment.
Stream is a reserved word in java and so that's why we tend to stick the word Airbyte in front of it. We will need to come up with a better name for this this thing. The and is not ideal, but we just don't have a better name yet.
What
Closes #1929
How
Implements option1 of #1924:
The intermediate layer StandardDataSchema in the back-end is now removed, we can directly convert an
AirbyteCatalogobject from the Airbyte Protocol into a newAirbyteCatalogobject generated from the API that reproduces almost the same fields/structures as the one from the Airbyte Protocol.The main difference is that the API doesn't make a distinction between an
AirbyteCatalogand aConfiguredAirbyteCatalog: The APIAirbyteCataloghas a defaultAirbyteStreamConfigurationfor each stream included.The new big change is that instead of storing an array of Fields, we now manipulate the JSON Schema objects directly, and thus, it can represent complex structures such as nested schema and arrays, etc
Note that:
dataType,cleanedName, andselectedfields for each fields by doing so. (could be added back inside the Json schema itself?)cleanedName, andselectedfields at the stream level thoughPre-merge Checklist
Recommended reading order
airbyte-api/src/main/openapi/config.yamlairbyte-server/src/main/java/io/airbyte/server/converters/SchemaConverter.java