[Low Code CDK] Pass DeclarativeStream's name into DefaultSchemaLoader
#21516
Conversation
clnoll
force-pushed
the
fix-missing-stream-name
branch
from
1ba7a5d to
98dd111
Compare
| def __post_init__(self, name: str, options: Mapping[str, Any]): | ||
| options = options or {} | ||
| if "name" not in options: | ||
| options["name"] = name |
There was a problem hiding this comment.
If name is defined, why not use self.name directly at https://github.com/airbytehq/airbyte/pull/21516/files#diff-eebfee995e9a963a52d517c0ebcab9579872dee179cf961719740e6c3fdddc9fR48 instead of self._options.get("name", "") that was generating the error? Do we need the options at all?
There was a problem hiding this comment.
name isn't available on self here, and at least for now for consistency with the rest of the factory we do want to override model.name if options is provided and includes it. Removing options is a possible refactor but felt out of scope for this PR (we've talked about doing a larger effort to remove it where it's not needed as there are various places where that's the case).
airbyte-cdk/python/airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Adding to my previous review:
Conditional approval: it feels like there is overlapping concerns with options and name and to avoid breaking the interface, I'm wondering if we could just have a different error message here when we don't have the name. If it would be possible to clean this overlapping concerns or just remove one of the fields, I think it would be great. In any case, I trust your judgement to change this or not
airbyte-cdk/python/airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
Show resolved
Hide resolved
| model_type=DeclarativeStreamModel, component_definition=propagated_source_config, config=input_config | ||
| ) | ||
| schema_loader = stream.schema_loader | ||
| assert schema_loader.default_loader._get_json_filepath().split("/")[-1] == f"{stream.name}.json" |
There was a problem hiding this comment.
Are we able to test the entire string, not just the final part just to be comprehensive?
Also handles the case where `DeclarativeStream.options` is `None`.
clnoll
force-pushed
the
fix-missing-stream-name
branch
from
98dd111 to
25d1893
Compare
Closes #21381
What
Fixes an issue where the CDK is not identifying the default schema location if a schema loader is not defined.
How
Problem: when
streamsis invoked fromairbyte/airbyte-cdk/python/airbyte_cdk/sources/declarative/manifest_declarative_source.py
Line 104 in 81be661
self._constructor.create_componentcreates aDeclarativeStreamobject; however, it is not created with theoptionsattribute. TheDeclarativeStreamobject creates aDefaultSchemaLoaderdownstream, which resolves to aJsonSchemaLoader.JsonSchemaLoaderattempts to identify the path to local stream schema files; it does this by reading the"name"key fromoptions. But, whenoptionsis not present, we return"", and so are attempting to find a file called.json, whereas we want the file name to default to<stream name>.json.This fixes the problem by passing
DeclarativeStream.nameto theDefaultSchemaLoader.