source-stripe: pass type checks #35587

alafanechere · 2024-02-23T15:55:28Z

What

Closes https://github.com/airbytehq/airbyte-internal-issues/issues/6349

We want to make mypy pass on a strategic connector. I picked source-stripe

How

Install mypy as a dev dependency
Run poetry run mypy source_stripe --disallow-untyped-defs
Fix the errors

It took me less than a day to make the type check pass.

Notes for reviewers

I'm unsure of 3 changes and would appreciate a connector expert some suggestion:

Here we call a method declared on HttpStream on a object typed as Stream. Can we make HttpAvailabilityStrategy handle HttpStream instead of Stream?
This code is dubious: we might call HttpStream.path but this method is not doing anything as it's not implemented in the abstract class.
math.inf (which is a float) was set for state_checkpoint_interval. I believe setting None is more correct but I'm not 💯 sure.

vercel · 2024-02-23T15:55:33Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 28, 2024 0:30am

alafanechere · 2024-02-23T15:55:41Z

source-stripe: pass type checks #35587 👈
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @alafanechere and the rest of your teammates on Graphite

maxi297 · 2024-02-23T16:36:21Z

HttpAvailabilityStrategy inherits from AvailabilityStrategy which expose the information in terms of Stream. So if we want to respect the typing and the Liskov principle, the conclusion is that our abstractions are not so great. From a quick look at this, I don’t get why the information provided by parse_response_error_message is not part of the exception that is raised. I don't know what would be the scope of changing that.
Does that mean that this path is never used and then we can raise here? I'm not entirely sure about that since it is marked as Optional here
None is indeed the right value as seen here

maxi297 · 2024-02-23T17:46:21Z

Diagram to document the issue with point 1:

Basically, we are trying to pass technology specific information though a technologic agnostic layer.

One proposal would be to use catch the technology specific errors in this layer and raise them as AirbyteTraceExceptions like this

maxi297

One concern about isinstance being used. For the rest, this is nit or me trying to understand things

maxi297 · 2024-02-28T16:44:55Z

airbyte-integrations/connectors/source-stripe/source_stripe/availability_strategy.py

@@ -29,8 +30,9 @@ def _check_availability_for_sync_mode(
        sync_mode: SyncMode,
        logger: logging.Logger,
        source: Optional["Source"],
-        stream_state: Optional[Mapping[str, Any]],
+        stream_state: Optional[Mapping[Any, Any]],


This typing change seems odd to me. Why is this needed? get_first_stream_slice should expect a Mapping[str, Any] and it seems like the only place this is used

maxi297 · 2024-02-28T16:48:52Z

airbyte-integrations/connectors/source-stripe/source_stripe/availability_strategy.py

@@ -90,7 +92,12 @@ def handle_http_error(
            raise error
        doc_ref = self._visit_docs_message(logger, source)
        reason = f"The endpoint {error.response.url} returned {status_code}: {error.response.reason}. {error_message}. {doc_ref} "
-        response_error_message = stream.parse_response_error_message(error.response)
+        # TODO alafanechere


We can't because of the inheritance we have in place right now. This comment describes the situation

maxi297 · 2024-02-28T16:49:39Z

airbyte-integrations/connectors/source-stripe/source_stripe/source.py

@@ -24,7 +23,7 @@
 from airbyte_cdk.sources.streams.concurrent.state_converters.datetime_stream_state_converter import EpochValueConcurrentStreamStateConverter
 from airbyte_cdk.sources.streams.http.auth import TokenAuthenticator
 from airbyte_cdk.utils.traced_exception import AirbyteTracedException
-from airbyte_protocol.models import SyncMode
+from airbyte_protocol.models import SyncMode  # type: ignore


Can we document why we have this type ignore? I'm unclear as to why we would ignore this

maxi297 · 2024-02-28T16:50:08Z

airbyte-integrations/connectors/source-stripe/source_stripe/source.py

+        self,
+        catalog: Optional[ConfiguredAirbyteCatalog],
+        config: Optional[Mapping[str, Any]],
+        state: Union[list[Any], MutableMapping[str, Any], None],


Why can't we keep TState here? I'm not very knowledgeable as to how MyPy handles TypeVar

maxi297 · 2024-02-28T16:50:54Z

airbyte-integrations/connectors/source-stripe/source_stripe/source.py

@@ -83,13 +88,14 @@ def __init__(self, catalog: Optional[ConfiguredAirbyteCatalog], config: Optional
            self._streams_configured_as_full_refresh = set()

    @staticmethod
-    def validate_and_fill_with_defaults(config: MutableMapping[str, Any]) -> MutableMapping[str, Any]:
+    def validate_and_fill_with_defaults(config: Mapping[str, Any]) -> Mapping[str, Any]:
+        mutable_config = dict(config)


Why can't MutableMapping allow for modifying the config? In practice, it is probably a dict anyway here

maxi297 · 2024-02-28T16:58:32Z

airbyte-integrations/connectors/source-stripe/source_stripe/streams.py

@@ -213,19 +219,20 @@ def get_parent_stream(self, stream_state: Mapping[str, Any]) -> StripeStream:

 class CreatedCursorIncrementalStripeStream(StripeStream):
    # Stripe returns most recently created objects first, so we don't want to persist state until the entire stream has been read
-    state_checkpoint_interval = math.inf
+    # TODO: alafanechere - confirm None is correct instead of math.inf
+    state_checkpoint_interval = None


I can confirm because of this

maxi297 · 2024-02-28T16:59:59Z

airbyte-integrations/connectors/source-stripe/source_stripe/streams.py

    ) -> MutableMapping[str, Any]:
        params = super(CreatedCursorIncrementalStripeStream, self).request_params(stream_state, stream_slice, next_page_token)
+        assert isinstance(stream_slice, dict), "stream_slice must be a dictionary"


I find this a bit dangerous as we could want to change this to any class that support the interface __get__ and this would start failing. Is there are reason why we need to validate that it is an instance of a dict?

maxi297 · 2024-02-28T17:03:33Z

airbyte-integrations/connectors/source-stripe/source_stripe/streams.py

@@ -511,15 +523,17 @@ def __init__(self, *args, **kwargs):
            start_date=self.start_date,
        )

-    def path(self, stream_slice: Mapping[str, Any] = None, **kwargs):
+    def path(self, stream_slice: Optional[Mapping[str, Any]] = None, **kwargs: Any) -> str:
+        assert isinstance(stream_slice, dict), "stream_slice must be a dictionary"


I find this a bit dangerous as we could want to change this to any class that support the interface __get__ and this would start failing. Is there are reason why we need to validate that it is an instance of a dict?

maxi297 · 2024-02-28T17:04:00Z

airbyte-integrations/connectors/source-stripe/source_stripe/streams.py

+                slice_ | rec
+                for rec in parent_records
+                for slice_ in incremental_slices
+                if isinstance(slice_, dict) and isinstance(rec, dict)


I find this a bit dangerous as we could want to change this to any class that support the interface __get__ and this would start failing. Is there are reason why we need to validate that it is an instance of a dict?

natikgadzhi · 2024-03-14T22:31:33Z

@Jgerardopine, would you like to take over the PR as an onboarding task for Eric? It will need more work, but I think the comments here are very useful for someone learning the architecture, and @maxi297 is the reviewer.

@alafanechere, hope that's fine by you!

lazebnyi

@alafanechere is this PR still relevant?

natikgadzhi · 2024-07-26T22:41:56Z

I doubt it. Tooling is not actively working on this, feel free to close or take over.

octavia-squidington-iii added the area/connectors Connector related issues label Feb 23, 2024

octavia-squidington-iii added area/documentation Improvements or additions to documentation connectors/source/stripe labels Feb 23, 2024

alafanechere force-pushed the augustin/02-23-source-stripe_pass_type_checks branch from c26d216 to 21c7982 Compare February 23, 2024 15:58

vercel bot deployed to Preview February 23, 2024 16:04 View deployment

alafanechere force-pushed the augustin/02-23-source-stripe_pass_type_checks branch from 21c7982 to ea5f477 Compare February 23, 2024 16:09

alafanechere marked this pull request as ready for review February 23, 2024 16:12

octavia-squidington-iv requested a review from a team February 23, 2024 16:13

vercel bot deployed to Preview February 23, 2024 16:14 View deployment

alafanechere force-pushed the augustin/02-23-source-stripe_pass_type_checks branch from f2af3c6 to 742f3fc Compare February 24, 2024 09:34

octavia-squidington-iv requested a review from a team February 26, 2024 17:13

alafanechere force-pushed the augustin/02-23-source-stripe_pass_type_checks branch from 532cb62 to 80301c2 Compare February 26, 2024 19:10

alafanechere changed the base branch from master to issue-35110/relax-cats-when-primary-key February 26, 2024 20:24

alafanechere requested review from a team as code owners February 26, 2024 20:24

alafanechere changed the base branch from issue-35110/relax-cats-when-primary-key to master February 26, 2024 20:25

alafanechere force-pushed the augustin/02-23-source-stripe_pass_type_checks branch from 80301c2 to 28290e0 Compare February 26, 2024 20:28

alafanechere requested review from lazebnyi and oustynova as code owners February 26, 2024 20:28

alafanechere force-pushed the augustin/02-23-source-stripe_pass_type_checks branch from 28290e0 to 8f97834 Compare February 26, 2024 21:19

alafanechere mentioned this pull request Feb 27, 2024

[ISSUE #35110] match CATs records only one primary key when primary k… #35556

Merged

alafanechere force-pushed the augustin/02-23-source-stripe_pass_type_checks branch from 89569e2 to 55b5330 Compare February 27, 2024 00:27

source-stripe: pass type checks

db92c9c

alafanechere force-pushed the augustin/02-23-source-stripe_pass_type_checks branch from 55b5330 to db92c9c Compare February 28, 2024 00:16

vercel bot deployed to Preview February 28, 2024 00:30 View deployment

maxi297 reviewed Feb 28, 2024

View reviewed changes

natikgadzhi assigned Jgerardopine Mar 14, 2024

postamar removed request for a team March 15, 2024 12:57

lazebnyi reviewed Jul 26, 2024

View reviewed changes

alafanechere closed this Aug 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

source-stripe: pass type checks #35587

source-stripe: pass type checks #35587

alafanechere commented Feb 23, 2024 •

edited

Loading

vercel bot commented Feb 23, 2024 •

edited

Loading

alafanechere commented Feb 23, 2024 •

edited

Loading

maxi297 commented Feb 23, 2024

maxi297 commented Feb 23, 2024

maxi297 left a comment

maxi297 Feb 28, 2024

maxi297 Feb 28, 2024

maxi297 Feb 28, 2024

maxi297 Feb 28, 2024

maxi297 Feb 28, 2024

maxi297 Feb 28, 2024

maxi297 Feb 28, 2024

maxi297 Feb 28, 2024

maxi297 Feb 28, 2024

natikgadzhi commented Mar 14, 2024

lazebnyi left a comment

natikgadzhi commented Jul 26, 2024 via email

source-stripe: pass type checks #35587

source-stripe: pass type checks #35587

Conversation

alafanechere commented Feb 23, 2024 • edited Loading

What

How

Notes for reviewers

vercel bot commented Feb 23, 2024 • edited Loading

alafanechere commented Feb 23, 2024 • edited Loading

maxi297 commented Feb 23, 2024

maxi297 commented Feb 23, 2024

maxi297 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natikgadzhi commented Mar 14, 2024

lazebnyi left a comment

Choose a reason for hiding this comment

natikgadzhi commented Jul 26, 2024 via email

alafanechere commented Feb 23, 2024 •

edited

Loading

vercel bot commented Feb 23, 2024 •

edited

Loading

alafanechere commented Feb 23, 2024 •

edited

Loading