Skip to content

fix: (CDK) (Manifest) - Add deprecations support and handle deprecation warnings; deprecate url_base, path, request_body_json and request_body_data for HttpRequester #486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Apr 29, 2025

Conversation

bazarnov
Copy link
Contributor

@bazarnov bazarnov commented Apr 16, 2025

What

Resolves:

How

Updated Component Models:

  • Modified the HttpRequester to support the new url field and prioritize it.
  • Updated schema definitions, validation logic, and deprecated fields in models.
  • Added logic to handle warnings during component creation (e.g., get_model_deprecations).
  • Ensure the backward compatibility with the deprecated url_base and path fields.

Handling Deprecation Warnings:

  • Added a new deprecation_warnings method to the DeclarativeSource and ManifestDeclarativeSource class to collect and process deprecation notices.
  • Updated the _categorise_groups() to handle deprecation warnings alongside message groups.
  • Added logic to collect these warnings in components using BaseModelWithDeprecations.
  • The new airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py module is added to cover the deprecation warnings for the fields marked as deprecated. This class is static (not autogenerated) and re-used when the declarative_component_schema.py is being generated. In this way, there is no need to set additional validations on top of the existing component implementations.

User Impact

  • No impact is expected, this is not a breaking change.

Deprecation Warning examples

The deprecation rules applied to the declarative_component_schema.yaml are automatically picked up and propagated to the declarative_component_schema.py (auto-gen models), see the example bellow.

# declarative component schema content

HttpRequester:
  required:
    - url
    # - url_base # removed from the required fields
  # .. the rest of the component content
  properties:
    url_base:
        deprecated: true
        deprecation_message: "Use `url` field instead."
        # .. the rest of the deprecated field content
    url:
       # .. the content of the deprecated field replacement
class HttpRequester(BaseModelWithDeprecations):
    type: Literal["HttpRequester"]
    url_base: Optional[str] = Field(
        None,
        deprecated=True, # deprecation flag is set onto the field
        deprecation_message="Use `url` field instead.", # deprecation message is set onto the field, will be used as a part of the warning.
        description="....",
        examples=[],
        title="API Base URL",
    )
    url: Optional[str] = Field(
        None,
        description="....",
        examples=[],
        title="API URL",
    )

Warning example

When using the deprecated field or creating an instance of the class with the deprecated field.

Python default behavior:

DeprecationWarning: Component type: `HttpRequester`. Field 'url_base' is deprecated. Use `url` field instead.

If multiple fields are marked as deprecated and used - more than 1 deprecation message is shown.

DeprecationWarning: Component type: `HttpRequester`. Field 'url_base' is deprecated. Use `url` field instead.

DeprecationWarning: Component type: `HttpRequester`. Field 'path' is deprecated. Use `url` field instead.

Connector Builder behavior:

Screenshot 2025-04-14 at 21 10 37

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Deprecation warnings for deprecated fields are now integrated and displayed in the Connector Builder UI.
    • Added support for a new url field in HTTP requesters, replacing the deprecated url_base and path fields.
    • Introduced a new model base class to automatically emit deprecation warnings for deprecated fields.
    • Enhanced component factory to collect and expose deprecation warnings from models.
    • Added a method to retrieve deprecation warnings from sources for improved user notifications.
    • Added unified request_body support for HTTP requests, replacing deprecated request_body_data and request_body_json.
  • Bug Fixes
    • Improved handling and aggregation of deprecation warnings to ensure clear user notifications.
  • Tests
    • Added coverage for deprecation warning scenarios in source mocks.
    • Added tests for request body interpolation using the new RequestBody model abstraction.
  • Chores
    • Enhanced code generation to detect and handle deprecated fields automatically in generated models.

@bazarnov bazarnov self-assigned this Apr 16, 2025
@github-actions github-actions bot added bug Something isn't working security labels Apr 16, 2025
@bazarnov bazarnov changed the title fix: (CDK) (Manifest) - Add deprecations support and deprecation warnings for deprecated manifest fields fix: (CDK) (Manifest) - Add deprecations support and handle deprecation warnings Apr 16, 2025
@bazarnov bazarnov marked this pull request as ready for review April 16, 2025 11:14
@Copilot Copilot AI review requested due to automatic review settings April 16, 2025 11:14
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Copy link
Contributor

coderabbitai bot commented Apr 16, 2025

📝 Walkthrough

Walkthrough

This update introduces a mechanism for handling deprecation warnings within Airbyte's declarative source framework. A new base model, BaseModelWithDeprecations, is added to emit warnings when deprecated fields are used and to collect these warnings as log messages. The component factory and source classes are extended to gather and expose deprecation warnings, which are then integrated into the test reading and result categorization processes. Code generation scripts are updated to replace base models with the new deprecation-aware model when deprecated fields are present. Mock classes in unit tests are also updated to support the new interface. Additionally, the HttpRequester component and related schema are updated to deprecate url_base and path in favor of a new url field, with corresponding changes to URL construction logic.

Changes

File(s) Change Summary
airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py Introduced BaseModelWithDeprecations to emit and collect deprecation warnings when deprecated fields are accessed or initialized.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py Enhanced ModelToComponentFactory to collect deprecation warnings from models, store them, and provide a method to retrieve these warnings. Adjusted HTTP requester creation to support new url field and dynamic URL retrieval in simple retriever creation.
airbyte_cdk/sources/declarative/declarative_source.py
airbyte_cdk/sources/declarative/manifest_declarative_source.py
Added deprecation_warnings method to both DeclarativeSource and ManifestDeclarativeSource classes, returning deprecation log messages from model deprecations.
airbyte_cdk/connector_builder/test_reader/reader.py Modified test reading logic to retrieve deprecation warnings from sources and integrate them into log message aggregation and categorization. Added comment on message group processing order.
bin/generate_component_manifest_files.py Added logic to replace base models with BaseModelWithDeprecations for classes containing deprecated fields during code generation, and inserted necessary imports. Updated code generation command to preserve deprecation metadata keys (deprecated, deprecation_message).
unit_tests/connector_builder/test_connector_builder_handler.py Updated mock source class in tests to implement the new deprecation_warnings method, returning an empty list.
airbyte_cdk/sources/declarative/declarative_component_schema.yaml Deprecated url_base and path properties in HttpRequester schema, added new url property as replacement, updated descriptions and deprecation messages accordingly. Deprecated request_body_data and request_body_json in favor of new request_body property with detailed schema.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py Changed HttpRequester to inherit from BaseModelWithDeprecations. Made url_base and path optional and deprecated with messages, added new optional url field. Added new RequestBody class and request_body field. Marked request_body_data and request_body_json as deprecated.
airbyte_cdk/sources/declarative/requesters/http_requester.py Added optional url attribute to HttpRequester. Updated URL construction to prioritize url or fallback to url_base and path. Added methods get_url and _get_url for flexible URL retrieval. Modified send_request to use new URL construction logic.
airbyte_cdk/sources/declarative/requesters/requester.py Added abstract method get_url to Requester interface to provide full base URL for requests.
airbyte_cdk/sources/declarative/interpolation/interpolated_nested_mapping.py Expanded NestedMapping type alias to include dict[str, Any] in addition to previous recursive types.
airbyte_cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.py Added optional request_body attribute to InterpolatedRequestOptionsProvider. Added method to resolve request_body into either request_body_data or request_body_json based on its type before interpolation.
unit_tests/sources/declarative/requesters/request_options/test_interpolated_request_options_provider.py Added parameterized tests to verify interpolation behavior when using RequestBody for JSON and data request bodies, covering nested and falsy values.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant TestReader
    participant DeclarativeSource
    participant ModelToComponentFactory
    participant BaseModelWithDeprecations

    User->>TestReader: run_test_read()
    TestReader->>DeclarativeSource: deprecation_warnings()
    DeclarativeSource->>ModelToComponentFactory: get_model_deprecations()
    ModelToComponentFactory->>BaseModelWithDeprecations: collect _deprecation_logs
    DeclarativeSource-->>TestReader: List[AirbyteLogMessage]
    TestReader->>TestReader: _categorise_groups(..., deprecation_warnings)
    TestReader-->>User: StreamRead (including deprecation warnings in logs)
Loading

Suggested labels

declarative-component-schema, manifest, migrations

Suggested reviewers

  • maxi297
  • bnchrch
  • lmossman

Would you like me to suggest adding some documentation or code comments to guide downstream connector developers on how to use or extend BaseModelWithDeprecations for their own deprecated fields? It might help adoption and clarity, wdyt?

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (1)

71-91: Overriding __getattribute__ may impact performance.

Overriding __getattribute__ is a powerful technique for intercepting attribute access, but it gets called for every attribute access which could impact performance. Have you considered using __getattr__ instead, which is only called when an attribute is not found through normal means? It would be less comprehensive but more performant.

Also, the exception handling seems to swallow all attribute errors, which might mask legitimate errors. Perhaps a more targeted exception handling approach would be better?

-    def __getattribute__(self, name: str) -> Any:
-        """
-        Show warnings for deprecated fields during field usage.
-        """
-
-        value = super().__getattribute__(name)
-
-        try:
-            model_fields = super().__getattribute__(FIELDS_TAG)
-            field_info = model_fields.get(name)
-            is_deprecated_field = (
-                field_info.field_info.extra.get(DEPRECATED, False) if field_info else False
-            )
-            if is_deprecated_field:
-                deprecation_message = field_info.extra.get(DEPRECATION_MESSAGE, "")
-                self._deprecated_warning(name, deprecation_message)
-        except (AttributeError, KeyError):
-            pass
-
-        return value
+    def __getattr__(self, name: str) -> Any:
+        """
+        Called only for attributes that don't exist on the instance,
+        so we don't need to do anything here - just raise the standard AttributeError.
+        """
+        raise AttributeError(f"{self.__class__.__name__} has no attribute '{name}'")
+
+    def __getattribute__(self, name: str) -> Any:
+        """
+        Show warnings for deprecated fields during field usage.
+        """
+
+        # Skip our special processing for dunder methods and our internal attributes
+        if name.startswith('__') or name == DEPRECATION_LOGS_TAG:
+            return super().__getattribute__(name)
+
+        value = super().__getattribute__(name)
+
+        # Only check for deprecation if this is a field defined in the model
+        try:
+            model_fields = super().__getattribute__(FIELDS_TAG)
+            if name in model_fields:
+                field_info = model_fields[name]
+                if field_info.field_info.extra.get(DEPRECATED, False):
+                    deprecation_message = field_info.field_info.extra.get(DEPRECATION_MESSAGE, "")
+                    self._deprecated_warning(name, deprecation_message)
+        except AttributeError:
+            # This happens when FIELDS_TAG isn't available, which is fine
+            pass
+
+        return value
bin/generate_component_manifest_files.py (1)

32-87: Solid implementation of base model replacement for deprecated fields.

The function looks great and serves its purpose well. It correctly identifies classes with deprecated fields and replaces their base model with BaseModelWithDeprecations.

Just noticed a tiny typo in the comment on line 62: "warinings" should be "warnings", wdyt?

Overall, great approach using regex to handle the class detection and replacement!

-  # update the imports to include the new base model with deprecation warinings
+  # update the imports to include the new base model with deprecation warnings
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)

746-757: Well-implemented deprecation collection methods

These new methods are clearly implemented:

  • get_model_deprecations() provides a public interface to access collected deprecation logs
  • _collect_model_deprecations() properly handles the collection of deprecation warnings with duplicate prevention

The implementation follows good practices with appropriate attribute checking and exclusion of duplicate entries.

Would it be clearer to add a type hint directly to the class attribute declaration rather than just in the initialization? Something like:

_deprecation_logs: List[AirbyteLogMessage] = []  # As a class attribute

This would make the type more visible when reviewing the class, wdyt?

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between bf998bd and ddfde29.

📒 Files selected for processing (7)
  • airbyte_cdk/connector_builder/test_reader/reader.py (5 hunks)
  • airbyte_cdk/sources/declarative/declarative_source.py (2 hunks)
  • airbyte_cdk/sources/declarative/manifest_declarative_source.py (2 hunks)
  • airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (1 hunks)
  • airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (4 hunks)
  • bin/generate_component_manifest_files.py (4 hunks)
  • unit_tests/connector_builder/test_connector_builder_handler.py (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
airbyte_cdk/sources/declarative/manifest_declarative_source.py (3)
airbyte_cdk/sources/declarative/declarative_source.py (1)
  • deprecation_warnings (41-45)
unit_tests/connector_builder/test_connector_builder_handler.py (1)
  • deprecation_warnings (821-822)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
  • get_model_deprecations (746-750)
airbyte_cdk/sources/declarative/declarative_source.py (2)
airbyte_cdk/sources/declarative/manifest_declarative_source.py (1)
  • deprecation_warnings (127-128)
unit_tests/connector_builder/test_connector_builder_handler.py (1)
  • deprecation_warnings (821-822)
unit_tests/connector_builder/test_connector_builder_handler.py (2)
airbyte_cdk/sources/declarative/declarative_source.py (1)
  • deprecation_warnings (41-45)
airbyte_cdk/sources/declarative/manifest_declarative_source.py (1)
  • deprecation_warnings (127-128)
airbyte_cdk/connector_builder/test_reader/reader.py (4)
airbyte_cdk/sources/declarative/declarative_source.py (1)
  • deprecation_warnings (41-45)
airbyte_cdk/sources/declarative/manifest_declarative_source.py (1)
  • deprecation_warnings (127-128)
unit_tests/connector_builder/test_connector_builder_handler.py (3)
  • deprecation_warnings (821-822)
  • primary_key (802-803)
  • cursor_field (806-807)
airbyte_cdk/connector_builder/models.py (1)
  • LogMessage (25-29)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (1)
  • BaseModelWithDeprecations (27-113)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Check: 'source-shopify' (skip=false)
🔇 Additional comments (22)
airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (6)

1-5: Nice explanatory comment about the purpose of this file.

The comment clearly explains that this is a static class model used for displaying deprecation warnings. Good practice to mention this upfront!


21-24: Clear constant definitions.

Using constants for field names and attributes makes the code more maintainable and less prone to typos. Good practice!


42-47: Config setting to allow extra fields.

Setting the extra = "allow" is a good choice here as it ensures models can have additional fields not explicitly defined, which is important for backward compatibility when dealing with deprecated fields.


51-70: Good implementation of checking for deprecated fields during initialization.

The method properly checks if initialized fields are marked as deprecated and triggers warnings accordingly. I like the use of the field's extra attributes to store deprecation metadata.


92-114: Solid implementation of warning method.

The method emits a warning using the standard Python warning system and also collects it as an Airbyte log message. Good documentation and clear parameter names.


16-19:

❓ Verification inconclusive

Global warning format looks good, but have you considered potential side effects?

Redefining the global warnings.formatwarning affects all warnings in the application, not just deprecation warnings from this model. Is this the intended behavior or would a more targeted approach be better, wdyt?


🏁 Script executed:

#!/bin/bash
# Check if warnings.formatwarning is modified elsewhere in the codebase
rg -A 1 "warnings\.formatwarning\s*=" --type py

Length of output: 299


Global warning format change may affect all warnings globally – can you confirm this behavior is intended, or would a more targeted approach be preferable?

I noticed that the global override of warnings.formatwarning in airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (lines 16–19) is the only instance in the codebase. This means any warning, not just deprecation warnings, will be formatted by this lambda. Could you verify if this global effect is desired, or would you consider scoping it (for example, using a context manager) to avoid unintended side effects? wdyt?

airbyte_cdk/sources/declarative/manifest_declarative_source.py (2)

18-26: Clean import addition for AirbyteLogMessage.

Adding AirbyteLogMessage to the existing imports from airbyte_cdk.models is tidy and follows the project's import style.


127-129: Well-implemented deprecation_warnings method.

The method follows a consistent pattern with other methods in the class, properly delegating to the constructor's get_model_deprecations() method. The fallback to an empty list is a good defensive programming practice.

airbyte_cdk/sources/declarative/declarative_source.py (2)

7-11: Clean import additions.

You've correctly added the necessary imports for the new method while maintaining the project's import style.


41-45: Good base implementation of deprecation_warnings.

Providing a default implementation that returns an empty list is a solid approach for the base class. This ensures that all subclasses have a consistent interface, even if they don't override this method.

unit_tests/connector_builder/test_connector_builder_handler.py (2)

10-10: Updated typing import to include List.

The import update ensures that List is available for the type annotation in the new method.


821-822: Added mock implementation for deprecation_warnings.

Your mock implementation matches the base class behavior, returning an empty list. This ensures tests will continue to work with the updated interface.

bin/generate_component_manifest_files.py (2)

103-105: Integration with post-processing looks good.

The function call to replace_base_model_for_classes_with_deprecated_fields is well-placed in the post-processing pipeline, ensuring that all generated code gets the appropriate base class replacements before being written to disk.


141-146: Great addition of field extra keys for deprecation support.

The command-line arguments for datamodel-codegen are well-documented with clear comments explaining their purpose. This enables the code generator to preserve the deprecation metadata from the YAML schema files.

airbyte_cdk/connector_builder/test_reader/reader.py (4)

116-117: Good addition of deprecation warnings retrieval.

This line properly connects to the new deprecation_warnings() method on the source, supporting the broader feature of collecting and exposing deprecation warnings.


133-133: Passing deprecation warnings to _categorise_groups looks good.

Simple but effective update to pass the newly retrieved deprecation warnings to the method for processing.


246-250: Well-typed parameter addition for deprecation warnings.

The optional parameter is properly typed and has an appropriate default value. This ensures backward compatibility while enabling the new functionality.


311-321: Clean implementation of deprecation warning processing.

The new code follows the existing pattern for processing message groups, which is great for consistency. I like the use of Python's pattern matching feature for type checking.

The error handling for unknown types of deprecation warnings matches the existing error handling for unknown message group types, maintaining consistency in the codebase.

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

30-30: Import addition looks good

Adding AirbyteLogMessage to the imports is necessary for handling the new deprecation logging functionality.


111-114: Clean imports for new deprecation functionality

The imports for the new deprecation-related components are clearly organized and properly separated in their own import statement.


591-592: Good initialization of deprecation logs list

Adding _deprecation_logs as a class attribute initialized with an empty list in the constructor is the right approach. The comment provides helpful context.


739-743: Proper handling of model deprecations

The code correctly checks if the model is an instance of BaseModelWithDeprecations before attempting to collect deprecation logs. This maintains compatibility with existing models while adding support for the new deprecation-aware models.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between ddfde29 and a308674.

📒 Files selected for processing (3)
  • airbyte_cdk/sources/declarative/manifest_declarative_source.py (2 hunks)
  • airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (1 hunks)
  • airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (4 hunks)
✅ Files skipped from review due to trivial changes (1)
  • airbyte_cdk/sources/declarative/manifest_declarative_source.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
⏰ Context from checks skipped due to timeout of 90000ms (9)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
  • GitHub Check: Pytest (Fast)
  • GitHub Check: SDM Docker Image Build
  • GitHub Check: Analyze (python)
🔇 Additional comments (3)
airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (3)

49-70: LGTM! Good initialization handling for deprecated fields

The implementation properly initializes _deprecation_logs as an instance attribute (addressing previous review comments) and correctly checks for deprecated fields during component initialization.


17-19: Clean warning format customization

Nice simplification of the warning format that focuses on what's important - the category and message, without the noisy file/line information.


91-112: The warning message implementation looks good

The method properly emits warnings through Python's built-in system and collects them in the instance's log collection. This dual approach ensures visibility both in standard output and in the Connector Builder UI.

@bazarnov bazarnov changed the title fix: (CDK) (Manifest) - Add deprecations support and handle deprecation warnings fix: (CDK) (Manifest) - Add deprecations support and handle deprecation warnings; deprecate url_base and path for HttpRequester Apr 16, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (6)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)

4184-4184: Missing newline at end of file.

The static analysis tool detected that there's no newline character at the end of the file. While this won't affect functionality, it's a common convention to have files end with a newline. Would you like to add one, wdyt?

- '{{ "goodbye, cruel world" | regex_search("goodbye,\s(.*)$") }} -> "cruel world"'
+ '{{ "goodbye, cruel world" | regex_search("goodbye,\s(.*)$") }} -> "cruel world"'
+ 
🧰 Tools
🪛 YAMLlint (1.35.1)

[error] 4184-4184: no new line character at the end of file

(new-line-at-end-of-file)

airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)

2173-2285: Consider adding a code example for the new URL approach in the documentation.

The field documentation is clear, but would it be helpful to add a short example in the class docstring that shows how to migrate from url_base + path to the new unified url approach? This could make it even easier for developers to understand the migration path, wdyt?

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

111-114: Minor import consolidation question.

Imports from base_model_with_deprecations look straightforward. Would you consider grouping them in a single line for cleanliness, or are you good with this layout? wdyt?


591-592: Optional data structure improvement.

Storing deprecation logs in a list works but checking for duplicates can be O(n). Would you consider using a set to handle membership checks if these logs grow large? wdyt?


746-750: Refine return type for deprecation retrieval?

Currently returns List[Any]. Would you consider specifying List[AirbyteLogMessage] since you are returning log messages? wdyt?


752-766: Optimize duplicate detection loop if many logs are expected.

Checking if a log is in _collected_deprecation_logs runs in O(n) time with a list. If performance becomes a concern, how about using a set for constant-time membership checks? wdyt?

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between f63f195 and 8dd1200.

📒 Files selected for processing (6)
  • airbyte_cdk/sources/declarative/declarative_component_schema.yaml (3 hunks)
  • airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (1 hunks)
  • airbyte_cdk/sources/declarative/models/declarative_component_schema.py (5 hunks)
  • airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (8 hunks)
  • airbyte_cdk/sources/declarative/requesters/http_requester.py (6 hunks)
  • airbyte_cdk/sources/declarative/requesters/requester.py (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
airbyte_cdk/sources/declarative/requesters/requester.py (2)
airbyte_cdk/sources/declarative/requesters/http_requester.py (1)
  • get_url (130-143)
airbyte_cdk/sources/types.py (1)
  • StreamSlice (66-160)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (1)
  • BaseModelWithDeprecations (27-122)
airbyte_cdk/sources/declarative/requesters/http_requester.py (1)
  • HttpRequester (37-478)
🪛 YAMLlint (1.35.1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml

[error] 4184-4184: no new line character at the end of file

(new-line-at-end-of-file)

⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: SDM Docker Image Build
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Pytest (Fast)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (20)
airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (1)

27-123: Impressive implementation of the deprecation handling system!

The BaseModelWithDeprecations class is well-designed with clear documentation and separation of concerns. The approach of emitting both standard Python warnings and collecting custom Airbyte log messages is elegant and practical.

Just one thought - I notice you're initializing _deprecation_logs as an instance variable in the constructor, which is great. This addresses the shared state concern that was raised in a previous review. The implementation looks solid now!

airbyte_cdk/sources/declarative/requesters/requester.py (1)

37-48: LGTM! Clean addition of the get_url abstract method.

The method signature is well-defined with appropriate type annotations and the docstring clearly explains its purpose. This addition provides a good abstraction for URL handling that can be implemented by concrete requester classes.

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

1871-1890: Good approach for deprecating url_base while maintaining backward compatibility.

The deprecation is clearly marked with an informative message pointing users to the new url field. The description update properly informs users that this field is deprecated.


1891-1909: Well-structured replacement url property with matching capabilities.

The new url property maintains all the interpolation contexts and examples from url_base, ensuring a smooth transition for users. Nice implementation!


1911-1914: Consistent approach with the path deprecation.

The deprecation message for path consistently directs users to use the url field instead, which aligns well with the overall approach.

airbyte_cdk/sources/declarative/requesters/http_requester.py (6)

58-60: Looks like a clean approach to field deprecation!

Adding the new optional url field while keeping url_base allows for backward compatibility. This graceful transition will help users to migrate their existing connectors at their own pace.


73-84: Good implementation of the deprecation transition.

The __post_init__ logic handles both new and deprecated fields appropriately, and I appreciate that you added clear comments to indicate which fields are deprecated. This helps other developers understand the code evolution.


130-144: Well-implemented new get_url method.

This method follows the same pattern as other getters in the class and properly handles the interpolation context for the URL template. Good work on maintaining consistency with the existing codebase style.


145-174: Solid implementation of URL resolution logic.

The _get_url method nicely handles the different URL construction scenarios with a clear priority order: using url_base + path if url_base exists, otherwise using url + path if path exists, or just url. This ensures backward compatibility while supporting the new approach.


404-414: Good update to the _join_url method.

Making the path parameter optional is consistent with the overall deprecation approach, and the documentation has been updated to reflect this change.


454-459: Nice refactoring of the URL construction in send_request.

Using the new _get_url method here centralizes the URL construction logic, which makes the code more maintainable and consistent. Good example of the DRY principle.

airbyte_cdk/sources/declarative/models/declarative_component_schema.py (7)

11-13: Good addition of the new import.

The import of BaseModelWithDeprecations is properly organized, and it's a good practice to have the deprecation mechanism in a separate module for better code organization.


885-897: Type field added and field optionality improved for KeyTransformation.

The required type field and making prefix and suffix optional with improved type hints will help with model validation. This should provide better flexibility for users.


918-922: Consistent update to the key_transformation field type.

Making this field optional aligns with the changes to the KeyTransformation class. Good attention to detail in ensuring consistency across model relationships.


2173-2174: Great choice of base class for enabling deprecation warnings.

Changing the base class to BaseModelWithDeprecations is the key change that enables the whole deprecation warning mechanism. This is an elegant way to add this functionality without affecting existing code too much.


2175-2187: Properly structured deprecation of url_base.

The field is now optional, correctly marked as deprecated, and includes a helpful message pointing users to the new url field. The description has also been updated to indicate it's deprecated.


2188-2198: Well-documented new url field.

The new field has comprehensive documentation and examples that match the style of the previous url_base field. This will make it clear to users how to migrate to the new field.


2199-2210: Properly structured deprecation of path.

Like with url_base, the path field is correctly marked as deprecated with an informative message directing users to the new approach. This consistency is important for a smooth transition.

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

30-30: Imports look good.

The addition of AirbyteLogMessage, FailureType, and Level appears consistent with the rest of the codebase. No issues spotted.


740-742: Conditional call to collect deprecations.

This is a solid approach for automatically collecting deprecation logs. No concerns here.

Oleksandr Bazarnov added 2 commits April 16, 2025 18:08
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (2)

17-19: Consider the implications of globally modifying warning format

This change to warnings.formatwarning affects all warnings in the application, not just the ones from this class. Would it make more sense to scope this to only your deprecation warnings by using a custom warning display function instead of modifying the global one, wdyt?

-warnings.formatwarning = (
-    lambda message, category, *args, **kwargs: f"{category.__name__}: {message}"
-)
+def _display_deprecation_warning(message: str, category: Type[Warning], *args, **kwargs) -> None:
+    formatted_message = f"{category.__name__}: {message}"
+    # Use the warnings module to emit the warning, but with custom formatting
+    warnings._showwarnmsg_impl(warnings.WarningMessage(
+        message=message, category=category, filename=args[0], 
+        lineno=args[1], file=args[2], line=args[3]
+    ))

Or at minimum, add a comment explaining the global modification and its potential impact on other parts of the codebase.


129-131: Consider optimizing duplicate detection

The current approach compares entire AirbyteLogMessage objects to check for duplicates. For large numbers of deprecation warnings, would it be more efficient to compare just the message strings instead, wdyt?

-        if deprecation_log_message not in self._deprecation_logs:
+        # Check if this message text already exists in the logs
+        if not any(log.message == deprecation_log_message.message for log in self._deprecation_logs):
             # Avoid duplicates in the deprecation logs
             self._deprecation_logs.append(deprecation_log_message)
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between baf1058 and c94363e.

📒 Files selected for processing (1)
  • airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: Analyze (python)
🔇 Additional comments (2)
airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (2)

49-60: The initialization order looks good

The implementation looks solid - you've addressed the previous concern about the class-level _deprecation_logs attribute by initializing it at the instance level in __init__. The order of operations (initializing Pydantic internals first, then processing fields) makes sense.


1-132: Overall implementation is robust and well-documented

This class provides a solid foundation for handling deprecation warnings in declarative components. I particularly like how thorough the docstrings are and the dual approach of emitting both Python warnings and Airbyte log messages.

One thing to consider: this implementation warns about deprecated fields but doesn't actually handle migration logic. For fields that are deprecated in favor of new ones (like url_base/pathurl), would it make sense to add automatic migration logic to maintain backward compatibility while still encouraging the new field usage, wdyt?

Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a couple small thoughts on the declarative_component_schema - will leave python review to @maxi297 or someone else on the python side

@bazarnov bazarnov requested a review from lmossman April 21, 2025 20:13
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (3)

1890-1907: Validate the new url field definition
The url property neatly replaces the url_base+path combo, and the interpolation contexts/examples look consistent. Do we need to update any spec documentation or add it to a required block when both url_base and path are absent? wdyt?


1909-1913: Ensure path deprecation consistency
path is also deprecated in favor of url, matching the url_base pattern. Would it be helpful to include a migration example in the deprecation_message or description to guide users through updating manifests? wdyt?


4182-4182: Add newline at end-of-file
YAMLlint flagged a missing newline at EOF. Could you add a blank line after line 4181 to satisfy the linter? wdyt?

🧰 Tools
🪛 YAMLlint (1.35.1)

[error] 4182-4182: no new line character at the end of file

(new-line-at-end-of-file)

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 9f743f2 and 97c83d9.

📒 Files selected for processing (1)
  • airbyte_cdk/sources/declarative/declarative_component_schema.yaml (3 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml

[error] 4182-4182: no new line character at the end of file

(new-line-at-end-of-file)

⏰ Context from checks skipped due to timeout of 90000ms (9)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: SDM Docker Image Build
  • GitHub Check: Pytest (Fast)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
  • GitHub Check: Analyze (python)
🔇 Additional comments (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)

1871-1874: Confirm url_base deprecation alignment
I see url_base is now marked as deprecated with a clear deprecation_message and updated description—nice work! Should we double‑check that it’s been removed from any required lists or external docs to avoid confusion? wdyt?

Copy link
Contributor

@maxi297 maxi297 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple of questions to make sure I understand but overall, it looks good

Copy link
Contributor

@maxi297 maxi297 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other concerns: I don't see any tests for that. Should we add some?

Copy link
Contributor

@maxi297 maxi297 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One (hopefully) last question: there were things that were deprecated in the past like RequestOption.field_name. Is there a plan to use this deprecation warning for these too? I unfortunately don't have a clear list of those though...

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (5)
airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (2)

56-58: Consider moving instance variables to type hints.

Would it be clearer to declare these instance variables with type annotations at the class level, even though you're initializing them in __init__? This would make the class interface more explicit, wdyt?

class BaseModelWithDeprecations(BaseModel):
+    _default_deprecation_messages: List[str]
+    _deprecation_logs: List[AirbyteLogMessage]
    
    class Config:

105-117: Consider clarifying attribute purpose with a more descriptive name.

The method _set_deprecation_logs_attr_to_model is setting an attribute using setattr, but the purpose might not be immediately clear. Is the attribute being set for access by external code, or for internal use? Maybe a more descriptive name would help, wdyt?

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

1907-1912: Suggest adding URL format validation
To ensure clients provide valid URLs, could we add format: uri under the url_base property? wdyt?

 url_base:
   deprecated: true
   deprecation_message: "Use `url` field instead."
   title: API Base URL
-  type: string
+  type: string
+  format: uri

1929-1930: Suggest adding URL format validation
Similarly, should we add format: uri to the new url field to enforce proper URL syntax? wdyt?

 url:
   title: The URL of an API endpoint
   description: The URL of the API source. ...
-  type: string
+  type: string
+  format: uri

1945-1948: Suggest enforcing path format
For the deprecated path property, could we add format: uri-reference or a pattern: '^/' to validate it's a valid URL path? wdyt?

 path:
   deprecated: true
   deprecation_message: "Use `url` field instead."
   title: URL Path
-  description: Deprecated, use the `url` instead. Path ... 
+  description: Deprecated, use the `url` instead. Path ...
+  format: uri-reference
📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 97c83d9 and 77f9b29.

📒 Files selected for processing (4)
  • airbyte_cdk/sources/declarative/declarative_component_schema.yaml (3 hunks)
  • airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (1 hunks)
  • airbyte_cdk/sources/declarative/models/declarative_component_schema.py (5 hunks)
  • airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • airbyte_cdk/sources/declarative/models/declarative_component_schema.py
  • airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
🧰 Additional context used
🪛 YAMLlint (1.35.1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml

[error] 4218-4218: no new line character at the end of file

(new-line-at-end-of-file)

⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Pytest (Fast)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
  • GitHub Check: SDM Docker Image Build
🔇 Additional comments (7)
airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (7)

1-25: Well-structured file header, imports, and constants.

The code includes proper imports, a custom warning formatter, and constants for field tags and deprecation metadata. The warning formatter is a nice touch to produce cleaner output.


27-48: BaseModelWithDeprecations class is well-documented and designed.

The class purpose and behavior are clearly explained in the docstring. Setting extra = "allow" in the Config makes sense for a model that needs to handle potentially deprecated fields.


49-65: Constructor initialization flow looks good.

I like how you initialize the parent class first, set up placeholders for messages and logs, and then process the fields in a clear sequence.


66-81: Helper methods have good fallback behavior.

The _is_deprecated_field and _get_deprecation_message methods nicely handle the case when a field isn't in __fields__.


118-139: Good handling of duplicate warnings.

I appreciate the checks for duplicate messages in both self._default_deprecation_messages and self._deprecation_logs to prevent showing the same warning multiple times.


137-138: Potential concern with duplicate warnings between instances.

Since deprecation_log_message not in self._deprecation_logs only checks for duplicates within a single instance, is there a risk of showing duplicate warnings across different instances of the same model type with the same deprecated fields? Or is this handled at a higher level in the model_to_component._collect_model_deprecations()?


140-146: Warning emission is simple and effective.

The method to emit warnings is clean and straightforward. Using Python's built-in warning system is a good choice here.

Copy link
Contributor

@maxi297 maxi297 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for tackling the changes

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (3)

1-1: Copyright year needs to be updated

The copyright notice has the year 2025, but the current year is 2024. Would it make sense to update this to the current year, wdyt?

-# Copyright (c) 2025 Airbyte, Inc., all rights reserved.
+# Copyright (c) 2024 Airbyte, Inc., all rights reserved.

63-78: Consider optimizing the field access pattern for readability and performance

The _is_deprecated_field and _get_deprecation_message methods have similar patterns of accessing field information and checking if a field exists. Maybe we could extract the common field access logic to reduce duplication and improve performance by avoiding multiple dictionary lookups?

-    def _is_deprecated_field(self, field_name: str) -> bool:
-        return (
-            self.__fields__[field_name].field_info.extra.get(DEPRECATED, False)
-            if field_name in self.__fields__.keys()
-            else False
-        )
-
-    def _get_deprecation_message(self, field_name: str) -> str:
-        return (
-            self.__fields__[field_name].field_info.extra.get(
-                DEPRECATION_MESSAGE, "<missing_deprecation_message>"
-            )
-            if field_name in self.__fields__.keys()
-            else "<missing_deprecation_message>"
-        )
+    def _get_field_extra(self, field_name: str):
+        """Helper method to get field extra information if the field exists."""
+        if field_name in self.__fields__:
+            return self.__fields__[field_name].field_info.extra
+        return {}
+        
+    def _is_deprecated_field(self, field_name: str) -> bool:
+        """Check if a field is marked as deprecated."""
+        return self._get_field_extra(field_name).get(DEPRECATED, False)
+        
+    def _get_deprecation_message(self, field_name: str) -> str:
+        """Get the deprecation message for a field."""
+        return self._get_field_extra(field_name).get(
+            DEPRECATION_MESSAGE, "<missing_deprecation_message>"
+        )

What do you think about this refactoring approach?


123-123: Consider adding more context to the deprecation message

The warning message is informative, but it might be helpful to add a reference to where users can find more information about the deprecation or migration guidelines. Maybe add a link to documentation or a note about where to look for alternatives?

-        deprecated_message = f"Component type: `{self.__class__.__name__}`. Field '{field_name}' is deprecated. {message}"
+        deprecated_message = f"Component type: `{self.__class__.__name__}`. Field '{field_name}' is deprecated. {message} See our documentation for migration guidelines."

What do you think? Is there documentation we could point to?

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 77f9b29 and b4cdf55.

📒 Files selected for processing (5)
  • airbyte_cdk/connector_builder/test_reader/reader.py (3 hunks)
  • airbyte_cdk/sources/declarative/declarative_source.py (2 hunks)
  • airbyte_cdk/sources/declarative/manifest_declarative_source.py (2 hunks)
  • airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (1 hunks)
  • airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (8 hunks)
✅ Files skipped from review due to trivial changes (1)
  • airbyte_cdk/connector_builder/test_reader/reader.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • airbyte_cdk/sources/declarative/declarative_source.py
  • airbyte_cdk/sources/declarative/manifest_declarative_source.py
  • airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (2)
airbyte_cdk/connector_builder/models.py (1)
  • LogMessage (25-29)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (16)
  • Config (134-135)
  • Config (148-149)
  • Config (162-163)
  • Config (176-177)
  • Config (194-195)
  • Config (208-209)
  • Config (222-223)
  • Config (236-237)
  • Config (250-251)
  • Config (264-265)
  • Config (278-279)
  • Config (292-293)
  • Config (308-309)
  • Config (322-323)
  • Config (336-337)
  • Config (370-371)
⏰ Context from checks skipped due to timeout of 90000ms (9)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Analyze (python)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
  • GitHub Check: Pytest (Fast)
  • GitHub Check: SDM Docker Image Build
🔇 Additional comments (5)
airbyte_cdk/sources/declarative/models/base_model_with_deprecations.py (5)

79-101: Great implementation of the field processing logic

The field processing method is well-structured and the logic for identifying and handling deprecated fields is clear. I especially like how you've added comprehensive docstrings explaining the purpose and behavior.


115-138: Effective warning and log message handling

The implementation of warning creation and tracking is well done. I particularly like:

  1. The detailed warning message format with component type and field name
  2. The deduplication logic to avoid repeated warnings
  3. The dual approach of using both standard Python warnings and custom log messages

This provides good flexibility for different use cases.


139-145: Good implementation of warning emission

The method for emitting collected deprecation warnings is simple and effective. Using Python's built-in warnings module is a good choice for this use case.


46-62: Excellent initialization sequence

I appreciate how you've structured the initialization process:

  1. First calling the parent constructor
  2. Setting up instance-level variables for message tracking
  3. Processing fields and emitting warnings
  4. Setting attributes on the model

This approach ensures everything is properly initialized before checking for deprecations.


24-45: Well-documented base class with clear purpose

The class documentation clearly explains the purpose and behavior of the model. The Config class properly allows extra fields, which is consistent with other models in the codebase.

@bazarnov bazarnov changed the title fix: (CDK) (Manifest) - Add deprecations support and handle deprecation warnings; deprecate url_base and path for HttpRequester fix: (CDK) (Manifest) - Add deprecations support and handle deprecation warnings; deprecate url_base, path, request_body_json and request_body_data for HttpRequester Apr 28, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (2)

4073-4092: Define RequestBody schema for payloads
The new RequestBody definition captures both JSON and data payload types. To ensure both fields are always provided, maybe consider adding a required: [type, value] constraint? wdyt?


4301-4301: Add newline at end of file
YAMLlint flagged a missing newline at EOF. Could we append a blank line to satisfy the new-line-at-end-of-file rule? wdyt?

🧰 Tools
🪛 YAMLlint (1.35.1)

[error] 4301-4301: no new line character at the end of file

(new-line-at-end-of-file)

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 52d8a92 and 6ecf85d.

📒 Files selected for processing (8)
  • airbyte_cdk/sources/declarative/declarative_component_schema.yaml (7 hunks)
  • airbyte_cdk/sources/declarative/interpolation/interpolated_nested_mapping.py (1 hunks)
  • airbyte_cdk/sources/declarative/manifest_declarative_source.py (2 hunks)
  • airbyte_cdk/sources/declarative/models/declarative_component_schema.py (7 hunks)
  • airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (9 hunks)
  • airbyte_cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.py (4 hunks)
  • unit_tests/connector_builder/test_connector_builder_handler.py (2 hunks)
  • unit_tests/sources/declarative/requesters/request_options/test_interpolated_request_options_provider.py (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • airbyte_cdk/sources/declarative/manifest_declarative_source.py
  • unit_tests/connector_builder/test_connector_builder_handler.py
  • airbyte_cdk/sources/declarative/models/declarative_component_schema.py
  • airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
airbyte_cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.py (2)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
  • RequestBody (1502-1504)
airbyte_cdk/sources/streams/http/http.py (2)
  • request_body_data (212-227)
  • request_body_json (229-240)
🪛 YAMLlint (1.35.1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml

[error] 4301-4301: no new line character at the end of file

(new-line-at-end-of-file)

⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
  • GitHub Check: Pytest (Fast)
🔇 Additional comments (14)
airbyte_cdk/sources/declarative/interpolation/interpolated_nested_mapping.py (1)

15-15: Nicely broadened the NestedMapping type signature.

The type signature has been expanded to include dict[str, Any], making it more flexible to handle a wider variety of data structures in nested mappings. This change aligns well with the needs for more complex request body handling.

airbyte_cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.py (4)

9-11: Clean import addition for the new RequestBody model.

Good job adding the import for the RequestBody model class which will be used for the new unified request body handling.


44-44: Good addition of the new request_body attribute.

This new optional attribute enables the unified request body handling that's being introduced. The type annotation correctly matches the imported RequestBody model.


54-57: Clear comments that explain the flow control.

The added comments explain the sequence of request body resolution nicely, making the code more maintainable for future developers.


80-94: Well-implemented request body resolution method.

This method provides a clean implementation for resolving the request body configuration. It appropriately handles the two possible types ("RequestBodyData" and "RequestBodyJson") and sets the corresponding attribute.

The docstring clearly explains the method's purpose, behavior, and potential errors. This is very helpful for maintainability.

unit_tests/sources/declarative/requesters/request_options/test_interpolated_request_options_provider.py (3)

7-7: Good import of the RequestBody model for testing.

The import is correctly added to support the new tests for the RequestBody functionality.


135-198: Comprehensive test coverage for JSON request body handling.

This test thoroughly verifies that JSON request bodies wrapped in the new RequestBody model work correctly with interpolation. It reuses the same test cases as the direct request_body_json tests, ensuring consistent behavior between the old and new approaches.

The test initialization correctly uses the RequestBody model with the "RequestBodyJson" type, and properly verifies the interpolation results.


238-278: Thorough test coverage for data request body handling.

Similar to the JSON test, this test verifies that data request bodies wrapped in the RequestBody model work correctly. It maintains consistency by reusing the same test cases as the direct request_body_data tests.

The test correctly initializes the RequestBody with the "RequestBodyData" type and verifies that interpolation works as expected.

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

1919-1924: Mark url_base as deprecated with clear guidance
Great to see url_base flagged as deprecated and pointing users to the new url field for endpoint definitions. wdyt?


1939-1956: Introduce url for full endpoint URLs
The new url property cleanly replaces the split url_base+path approach and includes proper interpolation context. Looks consistent and intuitive—wdyt?


1958-1963: Deprecate path in favor of url
Marking path as deprecated with a directional message helps maintain backward compatibility while steering users to the unified url. wdyt?


2003-2004: Deprecate request_body_data
Switching request_body_data to point at the new unified request_body field streamlines payload handling. wdyt?


2023-2024: Deprecate request_body_json
Similarly, deprecating request_body_json in favor of request_body is a nice consolidation. wdyt?


2043-2072: Add unified request_body field
Consolidating JSON and form-data into a single request_body union (RequestBody reference) simplifies the schema and improves clarity. wdyt?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (2)

1939-1957: Refine the url field description for clarity?
You've introduced the new url property perfectly. Could we tweak its description to explicitly mention that it expects the full API endpoint URL (not just the API source base), in line with UI usage patterns? wdyt?


4305-4305: Add newline at end of file
YAMLLint flagged a missing newline at EOF. Could we append a blank line to satisfy the new-line-at-end-of-file rule? wdyt?

🧰 Tools
🪛 YAMLlint (1.35.1)

[error] 4305-4305: no new line character at the end of file

(new-line-at-end-of-file)

📜 Review details

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

📥 Commits

Reviewing files that changed from the base of the PR and between 6ecf85d and 5d2d86b.

📒 Files selected for processing (6)
  • airbyte_cdk/manifest_migrations/migrations/registry.yaml (1 hunks)
  • airbyte_cdk/sources/declarative/declarative_component_schema.yaml (7 hunks)
  • airbyte_cdk/sources/declarative/manifest_declarative_source.py (2 hunks)
  • airbyte_cdk/sources/declarative/models/declarative_component_schema.py (7 hunks)
  • airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (9 hunks)
  • unit_tests/manifest_migrations/conftest.py (5 hunks)
✅ Files skipped from review due to trivial changes (2)
  • airbyte_cdk/manifest_migrations/migrations/registry.yaml
  • unit_tests/manifest_migrations/conftest.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • airbyte_cdk/sources/declarative/manifest_declarative_source.py
  • airbyte_cdk/sources/declarative/models/declarative_component_schema.py
  • airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
🧰 Additional context used
🪛 YAMLlint (1.35.1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml

[error] 4305-4305: no new line character at the end of file

(new-line-at-end-of-file)

⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: SDM Docker Image Build
  • GitHub Check: Pytest (Fast)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (5)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (5)

1919-1924: Deprecate url_base property
Nice work deprecating the old url_base field in favor of the consolidated url field! The deprecated: true flag and deprecation_message are clear and guide users smoothly.


1958-1973: Deprecate path property
The deprecation of path in favor of the unified url makes the schema more ergonomic, and the warning message is on point.


2007-2014: Deprecate request_body_data property
Great job marking request_body_data as deprecated with a clear replacement notice pointing to request_body.


2027-2034: Deprecate request_body_json property
Similarly, the request_body_json deprecation is implemented correctly and steers users towards the new request_body abstraction.


2047-2076: Introduce unified request_body property
The new request_body field elegantly unifies JSON and non-JSON payloads into a single abstraction—nice enhancement! 🥳

@bazarnov bazarnov enabled auto-merge (squash) April 29, 2025 10:59
@bazarnov bazarnov disabled auto-merge April 29, 2025 10:59
@bazarnov bazarnov merged commit b9f4736 into main Apr 29, 2025
26 of 28 checks passed
@bazarnov bazarnov deleted the baz/cdk/add-deprecations-module branch April 29, 2025 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants