unit_tests/source_declarative_manifest/resources/source_the_guardian_api/components.py (1)

21-24: Consider using snake_case for variable names to follow Python conventions

Variable names currPage and totalPages are using camelCase, which is less common in Python. Could we rename them to current_page and total_pages to match PEP 8 naming conventions? WDYT?

unit_tests/source_declarative_manifest/conftest.py (1)

13-20: Should we handle invalid hash_type inputs gracefully?

Currently, if an invalid hash_type is provided to hash_text, it raises a KeyError. Would it be better to handle this case and raise a more informative exception or provide a default behavior? WDYT?

airbyte_cdk/cli/source_declarative_manifest/_run.py (1)

174-178: LGTM! Consider enhancing the error message with the actual value?

The type validation is a great addition! Would you consider adding the actual value to the error message to make debugging easier? Something like:

-                f"but got type: {type(config['__injected_declarative_manifest'])}"
+                f"but got type: {type(config['__injected_declarative_manifest'])} with value: {config['__injected_declarative_manifest']!r}"

wdyt?

unit_tests/source_declarative_manifest/test_source_declarative_w_custom_components.py (2)

42-60: Consider adding edge cases to the test?

The basic functionality test looks good! Would you consider adding some edge cases like:

• Empty string input
• Invalid Python syntax
• Module with syntax errors
  wdyt?

---

89-134: Comprehensive integration test, but might need error cases

The happy path testing is thorough! Would you consider adding test cases for:

1. Missing or invalid checksums
2. Invalid manifest structure
3. Error handling during source creation

Also, the start_date truncation comment could be more descriptive about why 2 days is chosen:

-    # Truncate the start_date to speed up tests
+    # Truncate the start_date to 2 days ago to reduce API calls while still testing recent data

unit_tests/source_declarative_manifest/resources/source_the_guardian_api/manifest.yaml (2)

63-156: Consider reducing duplication in stream definitions?

I notice that base_stream and content_stream have identical configurations for incremental_sync, retriever, and their sub-components. Would it make sense to use YAML anchors and aliases to reduce this duplication? For example:

  base_stream: &base_stream_config
    incremental_sync:
      # ... existing config ...
    retriever:
      # ... existing config ...

  content_stream:
    <<: *base_stream_config
    schema_loader:
      # ... specific schema config ...

This would make the configuration more maintainable and less prone to drift. wdyt?

---

327-327: Consider tightening the date pattern regex?

The current pattern ^([1-9][0-9]{3})\-(0?[1-9]|1[012])\-(0?[1-9]|[12][0-9]|3[01])$ allows some invalid dates (like 2023-02-31). Would it be helpful to use a more restrictive pattern that validates actual calendar dates? wdyt?

Also applies to: 374-374

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

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

990-993: Consider adding error handling for missing components module.

The code assumes the components module will be available. Should we add more descriptive error messages for common failure scenarios? WDYT?

     custom_component_class = self._get_class_from_fully_qualified_class_name(
         full_qualified_class_name=model.class_name,
-        components_module=self._get_components_module_object(config=config),
+        components_module=self._get_components_module_object(config=config),
+    ) if hasattr(model, 'class_name') else None
+    
+    if not custom_component_class:
+        raise ValueError(f"Could not create custom component. Missing class_name in {model}")

---

1084-1107: Consider documenting security implications of executing code from configuration.

Based on the retrieved learnings, custom Python components are intentionally unrestricted to support unpredictable use cases. Should we add a docstring warning about the security implications? WDYT?

     def _get_components_module_object(
         config: Config,
     ) -> types.ModuleType:
-        """Get a components module object based on the provided config.
+        """Get a components module object based on the provided config and execute the code.
 
         If custom python components is provided, this will be loaded. Otherwise, we will
         attempt to load from the `components` module already imported.
+        
+        Warning:
+            This method executes Python code from configuration without restrictions.
+            Ensure that the configuration is from a trusted source as it has full access
+            to Python's capabilities including file operations and network access.
         """

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

Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#174
File: airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py:1063-1067
Timestamp: 2025-01-13T23:49:48.658Z
Learning: When implementing code execution from configuration, use a combination of security measures:
1. AST validation to detect dangerous operations
2. RestrictedPython or custom restricted execution environment
3. Limited builtins to prevent access to dangerous functions
4. Import hooks and global restrictions to prevent file/network access

Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#174
File: airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py:1063-1067
Timestamp: 2025-01-13T23:38:28.436Z
Learning: When executing Python code from configuration, implement security measures like AST validation, sandboxing, or restricted execution environments to prevent security vulnerabilities.

Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#174
File: airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py:1093-1102
Timestamp: 2025-01-14T00:20:32.310Z
Learning: In the `airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py` file, the strict module name checks in `_get_class_from_fully_qualified_class_name` (requiring `module_name` to be "components" and `module_name_full` to be "source_declarative_manifest.components") are intentionally designed to provide early, clear feedback when class declarations won't be found later in execution. These restrictions may be loosened in the future if the requirements for class definition locations change.

unit_tests/sources/declarative/test_manifest_declarative_source.py (2)

19-19: Would you consider expanding the comment to be more specific about why it's needed?

The current comment indicates it's needed for dynamic imports, but it might be helpful to explain which specific dynamic imports rely on this module, wdyt?

-import unit_tests.sources.declarative.external_component  # Needed for dynamic imports to work
+import unit_tests.sources.declarative.external_component  # Required for custom component dynamic imports in test_valid_manifest

---

268-271: LGTM! Consider extracting these assertions into a helper method?

The assertions are well-structured and verify the module loading hierarchy. To improve maintainability and reusability, would you consider extracting them into a helper method, wdyt?

+def assert_modules_loaded():
+    """Verify that all required modules are properly loaded."""
+    assert "unit_tests" in sys.modules
+    assert "unit_tests.sources" in sys.modules
+    assert "unit_tests.sources.declarative" in sys.modules
+    assert "unit_tests.sources.declarative.external_component" in sys.modules
+
 def test_valid_manifest():
     manifest = {
         # ... manifest configuration ...
     }
+    assert_modules_loaded()
-    assert "unit_tests" in sys.modules
-    assert "unit_tests.sources" in sys.modules
-    assert "unit_tests.sources.declarative" in sys.modules
-    assert "unit_tests.sources.declarative.external_component" in sys.modules

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

unit_tests/source_declarative_manifest/test_source_declarative_w_custom_components.py (3)

140-148: Consider using context manager for temporary file cleanup.

The temporary files created with delete=False aren't automatically cleaned up. Should we ensure cleanup using a try-finally block? Wdyt?

-    with NamedTemporaryFile(delete=False, suffix=".json") as temp_config_file:
+    temp_config_file = NamedTemporaryFile(delete=False, suffix=".json")
+    try:
         json_str = json.dumps(py_components_config_dict)
         Path(temp_config_file.name).write_text(json_str)
         temp_config_file.flush()
         source = create_declarative_source(
             ["check", "--config", temp_config_file.name],
         )
+    finally:
+        os.unlink(temp_config_file.name)

Also applies to: 174-182, 267-274

---

298-301: Specify exception type in except block.

The bare except could catch and mask important exceptions like KeyboardInterrupt. Should we specify the expected exception type? Wdyt?

-            with pytest.raises(Exception):
+            with pytest.raises((ValueError, RuntimeError)):  # specify expected exceptions
                 for msg in msg_iterator:
                     assert msg
             return

---

237-241: Consider creating a mock source for testing.

The TODO comment suggests creating a test source that doesn't require credentials. This would make the tests more reliable and faster. Would you like me to help create a mock source implementation?

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