airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)

1928-1934: ⚠️ Potential issue

Fix type check errors in the implementation

There are mypy errors in this section:

1. Line 1928: Type error when passing model.schema_loader to _create_component_from_model
2. Line 1934: Incompatible types in assignment (DefaultSchemaLoader vs CompositeSchemaLoader)

The first error is because mypy expects a BaseModel but gets a list. The second might be related to how the else branch assigns a DefaultSchemaLoader to a variable that previously held a CompositeSchemaLoader.

To fix the type issues, consider using a type annotation with Union for the schema_loader variable:

+        schema_loader: Union[CompositeSchemaLoader, DefaultSchemaLoader, JsonFileSchemaLoader, InlineSchemaLoader, DynamicSchemaLoader]
         if model.schema_loader and isinstance(model.schema_loader, list):
             nested_schema_loaders = [
                 self._create_component_from_model(model=schema_loader, config=config)
                 for schema_loader in model.schema_loader
             ]
             schema_loader = CompositeSchemaLoader(
                 schema_loaders=nested_schema_loaders, parameters=model.parameters or {}
             )
         elif model.schema_loader:
             schema_loader = self._create_component_from_model(
                 model=model.schema_loader, config=config
             )
         else:
             options = model.parameters or {}
             if "name" not in options:
                 options["name"] = model.name
             schema_loader = DefaultSchemaLoader(config=config, parameters=options)

🧰 Tools

🪛 GitHub Actions: Linters

[error] 1928-1928: mypy error: Argument "model" to "_create_component_from_model" of "ModelToComponentFactory" has incompatible type "InlineSchemaLoader | DynamicSchemaLoader | JsonFileSchemaLoader | CustomSchemaLoader | list[InlineSchemaLoader | DynamicSchemaLoader | JsonFileSchemaLoader | CustomSchemaLoader]"; expected "BaseModel" [arg-type]

---

[error] 1934-1934: mypy error: Incompatible types in assignment (expression has type "DefaultSchemaLoader", variable has type "CompositeSchemaLoader") [assignment]

airbyte_cdk/sources/declarative/declarative_component_schema.yaml (2)

1424-1427: Polish description grammar?
The new description is clear, but maybe we could tighten up the phrasing—for example, “Schema loaders defined first taking precedence…” could read better as “Loaders listed first take precedence in case of conflict.” Would you be up for a quick tweak here? wdyt?

---

1428-1439: Consider tightening the array schema constraints?
The addition of - type: array with items.anyOf nicely enables multiple loaders. A couple of ideas to guard against edge cases:

• Add minItems: 1 to ensure the array isn’t empty.
• Add uniqueItems: true to prevent duplicate loader definitions.
• Switch anyOf to oneOf if you want each value to match exactly one branch (single loader vs array) rather than any matching subset.

Any of these sound useful to you? wdyt?

airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)

1918-1926: Good implementation of multiple schema loader support, but consider passing model parameters

The implementation correctly handles both list and single schema loader cases, maintaining backward compatibility. However, you're creating the CompositeSchemaLoader with an empty parameters dictionary. Should you pass model.parameters or {} instead to be consistent with other component creations? wdyt?

-            schema_loader = CompositeSchemaLoader(
-                schema_loaders=nested_schema_loaders, parameters={}
-            )
+            schema_loader = CompositeSchemaLoader(
+                schema_loaders=nested_schema_loaders, parameters=model.parameters or {}
+            )

---

1918-1926: Consider adding a brief comment explaining the new functionality

It would be helpful to add a comment explaining the purpose of allowing multiple schema loaders and how they're combined. This makes the code more maintainable.

         if model.schema_loader and isinstance(model.schema_loader, list):
+            # Handle multiple schema loaders by creating a CompositeSchemaLoader
+            # which merges the properties from each schema loader, with earlier loaders taking priority
             nested_schema_loaders = [
                 self._create_component_from_model(model=schema_loader, config=config)
                 for schema_loader in model.schema_loader
             ]

unit_tests/sources/declarative/parsers/test_model_to_component_factory.py (1)

4567-4633: Good test case for multiple schema loaders functionality.

This test verifies that the factory correctly handles declarative streams defined with multiple schema loaders specified as a list. The test properly:

1. Defines a stream with two different inline schemas
2. Processes the manifest through parsing and transformation
3. Verifies the resulting schema loader is a CompositeSchemaLoader containing two InlineSchemaLoader instances

Have you considered adding a test case that mixes different schema loader types (like JsonFileSchemaLoader with InlineSchemaLoader) to verify that works too? Also, would it be worth adding an assertion to verify the actual schema content after merging? Just thinking about edge cases, wdyt?

airbyte_cdk/sources/declarative/schema/composite_schema_loader.py (2)

11-32: Clean implementation of the CompositeSchemaLoader!

The implementation is straightforward and the docstring clearly explains the precedence behavior. The merging of schemas is handled efficiently.

One thing to consider: the current implementation only merges top-level properties, but doesn't perform deep merging of nested properties. In most cases this is probably fine, but would a deep merge be beneficial for some use cases? For example:

def get_json_schema(self) -> Mapping[str, Any]:
    combined_schema: Dict[str, Any] = {
        "$schema": "http://json-schema.org/draft-07/schema#",
        "type": ["null", "object"],
        "properties": {},
    }
    for schema_loader in self.schema_loaders:
        schema_properties = schema_loader.get_json_schema()["properties"]
-        combined_schema["properties"] = {**schema_properties, **combined_schema["properties"]}
+        # Deep merge properties
+        self._deep_merge_properties(schema_properties, combined_schema["properties"])
    return combined_schema

+ def _deep_merge_properties(self, source: Dict[str, Any], target: Dict[str, Any]) -> None:
+     """Deep merge source properties into target, with target taking precedence."""
+     for key, value in source.items():
+         if key not in target:
+             target[key] = value
+         elif isinstance(value, dict) and isinstance(target[key], dict):
+             if "properties" in value and "properties" in target[key]:
+                 if "properties" not in target[key]:
+                     target[key]["properties"] = {}
+                 self._deep_merge_properties(value["properties"], target[key]["properties"])

Would that be useful? Or perhaps overkill for the current use cases?

---

29-30: Schema merging logic works but could be clearer!

The current implementation has properties from earlier loaders taking precedence, which is mentioned in the docstring. However, the dictionary unpacking order might be confusing at first glance.

Consider making the precedence more explicit with a comment or by rearranging the expression:

- combined_schema["properties"] = {**schema_properties, **combined_schema["properties"]}
+ # Ensure properties from earlier loaders take precedence
+ combined_schema["properties"] = {**schema_properties, **combined_schema["properties"]}

Or alternatively:

- combined_schema["properties"] = {**schema_properties, **combined_schema["properties"]}
+ # First apply new properties, then keep existing ones (which take precedence)
+ new_properties = dict(schema_properties)  # Make a copy
+ new_properties.update(combined_schema["properties"])  # Existing properties override new ones
+ combined_schema["properties"] = new_properties

This makes the intention clearer even to readers who aren't familiar with dictionary unpacking behavior. What do you think?

unit_tests/sources/declarative/schema/test_composite_schema_loader.py (3)

55-151: Great test for the dynamic/static schema combination!

This test thoroughly verifies that the CompositeSchemaLoader correctly combines properties from both static and dynamic schema loaders.

One suggestion - it might be helpful to add assertions that specifically verify the schema structure beyond just comparing the entire expected schema. For example:

# Verify specific aspects of the schema
assert "dynamic_property_one" in schema["properties"]
assert "id" in schema["properties"]
assert schema["properties"]["nestedDisplayOptions"]["properties"]["theme"]["type"] == ["null", "string"]

This would make it clearer what aspects of the schema are most important to validate and provide more specific failure messages if something goes wrong. What do you think?

---

153-211: Good test for property precedence behavior!

This test properly verifies that properties from earlier schema loaders take precedence over later ones.

Consider adding a comment explaining why certain properties appear the way they do in the expected schema. For example, a comment noting that "updatedAt" from the first loader takes precedence over the version in the second loader, and "id" from the first loader takes precedence over the version in the third loader. This would make the test's intent clearer to future readers.

# Test specifically verifies:
# - "updatedAt" from first loader (format: date-time) overrides second loader (format: date)
# - "id" from first loader (type: string) overrides third loader (type: integer) 
# - "organization" from second loader is included
expected_schema = {
    ...
}

Would that help clarify the test's purpose?

---

1-211: Consider adding tests for edge cases!

The tests cover the main functionality well, but there are a few edge cases that might be worth testing.

Consider adding tests for:

1. Empty schema_loaders list
2. Schema loader that returns a schema without properties
3. Schema loader that returns None or raises an exception

These would help ensure the CompositeSchemaLoader is robust against unexpected inputs. Would these additional tests be valuable?

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro