🪟🎉 Connector builder: Substream slicer and cartesian slicer

[flash1293]

What

This PR adds support for configuring substream slicers and cartesian product slicers to the connector builder UI:

Things I split out:

• Can't configure multiple parent stream configs in the substream slicer - it's not used in any existing connector and would complicate an already pretty complex piece of the UI further. IMHO we should go with a single parent stream for now and see whether someone requests this functionality
• With substream slicers it's possible to run into cyclic dependencies. In this case the form values -> manifest logic is simply returning an empty list for parent streams which results in a validation error on trying to list the streams:

The form validation logic should highlight the place where the cycle is formed, but with our current validation logic it's not possible to check for this case as a property validator function only has access to sibling properties, not parents/grandparents which would be necessary here. This can be changed, but I would like to keep this PR focused and it seems like an edge case.

How

• Add a uuid to each stream so they can be referenced in a stable way (indices would need to be updated on deleting streams which is more complex than maintaining this extra piece of state)
• Let the user choose the id of another stream in the slicer section
• On building the manifest, recursively build stream definitions in place of the reference (we could use memoization here to avoid stream configs to be rebuilt over and over but it felt like overkill for now as it's not common to have deep trees of stream definitions)

For the cartesian product slicer add a BuilderList component to manage a list of similar form elements in a general way.

Do not allow to nest cartesian product slicers into cartesian product slicers as it doesn't add extra functionality but looks very weird.

Note

as existing state objects stored in local storage do not have ids per stream, the substream slicer won’t work for them. However I don’t think it’s worth introducing additional logic for this special case at the current state of the project, IMHO it’s sufficient for now to add a comment to the release notes recommending users to “reset all” if they run into problems. I think we should not maintain strict backwards compatibility for now outside of not crashing the UI and start with this once it’s available on cloud. If you feel strongly about this we can also introduce logic for patching this case.

[lmossman]

Had a few callouts, but overall this seems to work well. Nice work!

This code is definitely starting to feel pretty complex, but I think that is just a result of it being a complex problem to solve

[lmossman]

I didn't understand what stream_slice_field is supposed to do or how it differs from parent_key based off of this description.

@brianjlai or @sherifnada could you help us understand what these fields mean so that we can add more helpful descriptions here?

[flash1293]

My understanding is that it names the field in the stream_slice object for interpolation (like this example getting all the moves from all the pokemon using a substream slicer):

I agree the naming is not 100% clear though...

[brianjlai]

Joe is correct. So the idea is that the parent_key is used to tell the slicer where to extract the key values from parent records which will then be stored in the slice at the location specified by stream_state_field for the substream. For example, imagine a parent stream w/ 1 slice of 3 records and parent_key = parent_id, stream_slice_field = some_id :

{ "parent_id": "123", "field": "val" }
{ "parent_id": "456", "field": "val" }
{ "parent_id": "789", "field": "val" }

Then on the substream slicer that we will will perform the partitioning into slices which will result in a total of 3 slices for the current substream:

{ "some_id": "123" }
{ "some_id": "456" }
{ "some_id": "789" }

And then when we make outbound requests for the substream, we can use the value in some_id to get all child records of that parent record.

[lmossman]

I don't think this is quite correct - it seems like path is allowed as the Inject into value for a Substream slicer, but not allowed for List or Datetime. So we will need to encode that here in the yup schema

[flash1293]

I checked the code and while the path injection is permitted it doesn't look like it has any effect (the slicer is simply ignoring it). This is how I tested:

version: 0.1.0
type: DeclarativeSource
check:
  type: CheckStream
  stream_names: []
streams:
  - type: DeclarativeStream
    name: moves
    primary_key: []
    retriever:
      type: SimpleRetriever
      name: moves
      primary_key: []
      requester:
        type: HttpRequester
        name: moves
        url_base: https://pokeapi.co/api/v2/
        path: >-
          pokemon//{{
          stream_slice.the_renamed_value_to_interpolate_somewhere_else }}
        request_options_provider:
          request_parameters: {}
          request_headers: {}
          request_body_json: {}
        authenticator:
          type: NoAuth
      record_selector:
        type: RecordSelector
        extractor:
          type: DpathExtractor
          field_pointer:
            - moves
      paginator:
        type: NoPagination
      stream_slicer:
        type: SubstreamSlicer
        parent_stream_configs:
          - type: ParentStreamConfig
            parent_key: name
            request_option:
              inject_into: path
              type: RequestOption
            stream_slice_field: the_renamed_value_to_interpolate_somewhere_else
            stream:
              type: DeclarativeStream
              name: pokemon
              primary_key: []
              retriever:
                type: SimpleRetriever
                name: pokemon
                primary_key: []
                requester:
                  type: HttpRequester
                  name: pokemon
                  url_base: https://pokeapi.co/api/v2/
                  path: pokemon/
                  request_options_provider:
                    request_parameters:
                      limit: '3'
                    request_headers: {}
                    request_body_json: {}
                  authenticator:
                    type: NoAuth
                record_selector:
                  type: RecordSelector
                  extractor:
                    type: DpathExtractor
                    field_pointer:
                      - results
                paginator:
                  type: NoPagination
spec:
  connection_specification:
    $schema: http://json-schema.org/draft-07/schema#
    type: object
    required: []
    properties: {}
    additionalProperties: true
  documentation_url: ''
  type: Spec

Also from the code I don't see any code actually using it, seems to be an oversight:

airbyte/airbyte-cdk/python/airbyte_cdk/sources/declarative/stream_slicers/substream_slicer.py

Line 35 in e029353

class SubstreamSlicer(StreamSlicer, JsonSchemaMixin):

The way I made it work is to use the stream_slice variable in jinja interpolation like you can see in the example manifest

Adjusted the PR to not list the path option

[flash1293]

Actually, it's even a bit worse than that, because it looks like for the other injection mechanisms it's ignoring the field name (always using the stream_slice_field), but it requires it by the schema definition:

airbyte/airbyte-cdk/python/airbyte_cdk/sources/declarative/stream_slicers/substream_slicer.py

Line 103 in e029353

key = parent_config.stream_slice_field

@brianjlai could you take a look at that? Seems like it should be cleaned up one way or another.

[flash1293]

For example for such a slicer:

I would expect the name from the parent record to be injected into the abc header, but it's actually injected into the the_renamed_value_to_interpolate_somewhere_else header:

{
  "url": "https://pokeapi.co/api/v2/pokemon/bulbasaur",
  "parameters": null,
  "body": null,
  "headers": {
    "User-Agent": "python-requests/2.28.1",
    "Accept-Encoding": "gzip, deflate",
    "Accept": "*/*",
    "Connection": "keep-alive",
    "the_renamed_value_to_interpolate_somewhere_else": "bulbasaur"
  },
  "http_method": "GET"
}

[flash1293]

Something we could do for now is to simply hide the request options completely for the substream slicer and require the user using jinja interpolation in the path or somewhere else (it looks like this is also how it's used in most cases in existing connectors, some examples):

• airbyte/airbyte-integrations/connectors/source-twilio-taskrouter/source_twilio_taskrouter/twilio_taskrouter.yaml

  Line 59 in 2a5d034

  path: "/v1/Workspaces/{{ stream_slice.id }}/Workers"

• airbyte/airbyte-integrations/connectors/source-woocommerce/source_woocommerce/woocommerce.yaml

  Line 146 in 2de1dca

  path: "/products/attributes/{{ stream_slice.id }}/terms"

• airbyte/airbyte-integrations/connectors/source-zoom/source_zoom/zoom.yaml

  Line 152 in 093a294

  path: "/meetings/{{ stream_slice.parent_id }}/polls"

• airbyte/airbyte-integrations/connectors/source-configcat/source_configcat/configcat.yaml

  Line 65 in 842a2e6

  path: "/products/{{ stream_slice.productId }}/environments"

[lmossman]

Agreed that hiding this in the UI makes sense for now

[brianjlai]

@flash1293 @lmossman

On the substream slicer you can set the request_option to inject into path, but it's not actually doing it (it's just ignored)

This is more or less a current limitation of the framework. Stream slicers don't have a concept of path since that is more of a requester concept. So if you want to hide it from the frontend that sounds fine to me because it's not being used.

Also, you can choose to inject into request_params, header and so on but that it is requiring the field name which is then ignored (it is using the stream_slice_field instead)

As you have pointed out in everything above, this to me looks to be a bug, we should be extracting the parent value from the stream_slice_field and storing it into params using the parent_config.request_option.field_name. I think the main reason we never caught this until now is that no customers are using substream request options except Braze. And for that their stream_slice_field and field_name had the same values.

I'm going to file a ticket to cover both these cases. For the first case, throwing an error when we see any parent_stream_configs that have path specified. And for the second case, we just need to change the mapping to use the field_name. Thanks for highlighting this.

[lmossman]

@brianjlai just curious, did we capture this in an issue? Want to make sure it doesn't go untracked

[brianjlai]

yep, I filed the ticket here: #21095

[lmossman]

To solve the circular dependency case that you called out, maybe we could expand this filter logic to also exclude streams that are descendants of the current stream.

I think this would require keeping track of the whole tree of substream parents in a state somewhere and will definitely add some complexity, so we can do that in a separate PR. Want to create an issue for that?

[flash1293]

Yeah, sounds good, gonna create an issue for that.

[lmossman]

Changes LGTM, tested locally and it seems to work as expected