Artem1205/lowcode cdk authenticator schemaloader #19718

artem1205 · 2022-11-22T16:01:18Z

What

#19753 - Declarative schema loader $ref
#19752 - Oauth Authenticator expiry date format

DeclarativeOauth2Authenticator expects expires_in in seconds (in get_access_token method). New feature makes it possible to specify date format (if not seconds)
Low-code connectors do not resolve schemas with references to another *.json files in shared folder, e.g. {"$ref": "file.json"} now. This PR fixes it.
- this functionality is moved from original (non low-code) CDK to deal with reference mechanism, which used shared folder to resolve '$ref' links.

How

Add token_expiry_date_format field to Declarative OAuth Authenticator.
Add Resolve ref schema

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

…tor-schemaloader' into artem1205/lowcode-cdk-authenticator-schemaloader # Conflicts: # airbyte-cdk/python/airbyte_cdk/sources/declarative/schema/json_file_schema_loader.py

airbyte-cdk/python/airbyte_cdk/sources/declarative/schema/json_file_schema_loader.py

airbyte-cdk/python/airbyte_cdk/sources/declarative/auth/oauth.py

sherifnada · 2022-11-29T05:15:56Z

@artem1205 apologies for the delay in review, will review tomorrow

brianjlai

Echoing Alex, it would be very helpful in the PR description to more concretely explain what the use case is for the reference resolution in the schema loader. Also had some notes around how the authenticator is being refactored

brianjlai · 2022-11-29T06:20:28Z

airbyte-cdk/python/airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py

@@ -29,10 +29,9 @@ def get_auth_header(self) -> Mapping[str, Any]:
    def get_access_token(self) -> str:
        """Returns the access token"""
        if self.token_has_expired():
-            t0 = pendulum.now()


one slight drawback to moving the now() generation into the authenticator set token expiry methods is that we do have a very small corner case where we may attempt a request with a token that has expired.

When we were using pendulum.now() before refreshing the access token, the corner case meant we might think a token is already expired and refresh it earlier than necessary.

However, now with the reverse of calling pendulum.now() after already refreshing the token, then at the very end of expiry, we may attempt a request where the token has already expired.

I'd argue that refreshing the token a little more often than necessary is better than attempting requests with an already expired token so just to be on the safe side, I think we should continue to get the current time before calling self.refresh_access_token() and pass it into self.set_token_expiry_date() Its a little verbose to pass both params in, but we're already changing the interface regardless.

airbyte-cdk/python/airbyte_cdk/sources/declarative/schema/json_file_schema_loader.py

brianjlai · 2022-11-29T06:38:07Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/schema/json_file_schema_loader.py

+        return resolved
+
+
+def resolve_ref_links(obj: Any) -> Union[Dict[str, Any], List[Any]]:


Can you add some unit tests to validate some each of the scenarios that can be recursed.

…loader

brianjlai

The oauth changes look good, but thinking about the references a little more, wanted to get your thoughts on a slight refactor of the file schema loader to reuse some more code.

brianjlai · 2022-11-30T00:26:51Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/schema/json_file_schema_loader.py

@@ -63,7 +68,8 @@ def get_json_schema(self) -> Mapping[str, Any]:
            raw_schema = json.loads(raw_json_file)
        except ValueError as err:
            raise RuntimeError(f"Invalid JSON file format for file {json_schema_path}") from err
-        return raw_schema
+        self.__resource = resource
+        return self._resolve_schema_references(raw_schema)


Given that we're mostly copying these two methods from the existing code in schema_helpers.py, it might be interesting to make the JsonFileSchemaLoader a subclass of ResourceSchemaLoader. And we can refactor the SchemaLoader interface to name get_json_schema() to get_schema(). And this class json_file_schema_loader would just be responsible for the overriding get_schema() and can reuse the dependent methods that were originally duplicated.

It does expand the scope of changes a little bit, but would help us cut down on the code duplication. Does this all make sense?

Makes sense, I've refactored it.

…_expiry_date_format)

brianjlai

At the moment, the way the method names are defined, we won't be properly invoking schema retrieval according to the declarative interface SchemaLoader. I mentioned a few alternatives in the comments

airbyte-cdk/python/airbyte_cdk/sources/declarative/schema/json_file_schema_loader.py

brianjlai · 2022-12-01T02:51:59Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/schema/json_file_schema_loader.py

@@ -50,7 +51,7 @@ def __post_init__(self, options: Mapping[str, Any]):
            self.file_path = _default_file_path()
        self.file_path = InterpolatedString.create(self.file_path, options=options)

-    def get_json_schema(self) -> Mapping[str, Any]:
+    def get_schema(self) -> Mapping[str, Any]:


If we are renaming get_json_schema() to get_schema() here, we will also need to rename it on the parent interface class SchemaLoader not just in DefaultSchemaLoader There are a few places where we use SchemaLoader.get_json_schema() at runtime so we need to properly override it so that it gets used everywhere.

Alternatively, if we want to simplify this, we could also leave this as get_json_schema(), so the parent interface doesn't have to change. And from the perspective of the declarative components we just don't reuse the parent class ResourceSchemaLoader.get_schema(self, name: str). And this still allows us to reuse the recursive resolver.

not sure what do mean about renaming, as I override get_schema which present in ResourceSchemaLoader.

@sherifnada , I would kindly ask you to review it.

@artem1205
JsonFileSchemaLoader also extends SchemaLoader so it needs to implement get_json_schema too

I've overwritten get_json_schema -> get_schema https://github.com/airbytehq/airbyte/pull/19718/files#diff-eebfee995e9a963a52d517c0ebcab9579872dee179cf961719740e6c3fdddc9fR40.
So we do not really need SchemaLoader anymore. I can remove it

I don't think we should be getting rid of SchemaLoader. This JsonFileSchemaLoader and the DefaultSchemaLoader are intended to be subclasses of the SchemaLoader. From within parts of the low-code CDK, we invoke this get_json_schema() at runtime. For example here: https://github.com/airbytehq/airbyte/blob/master/airbyte-cdk/python/airbyte_cdk/sources/declarative/declarative_stream.py#L132

If we rename this method in this class, we should also rename it in SchemaLoader as well instead of removing the interface altogether. And then we also rename wherever it's being used like in the file I mentioned. Just renaming this method name will probably break out backend because we're using it in a few different places.

✅ Refactored, renamed it all back to get_json_schema

…loader

brianjlai

Looks good to me and the latest merge of master looks to have fixed some of the unrelated test failures. Let just address the two formatting issues that are failing the CI checks and then I'll approve along with the CDK version bump.

brianjlai · 2022-12-13T20:50:34Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/schema/json_file_schema_loader.py

@@ -1,7 +1,6 @@
 #
 # Copyright (c) 2022 Airbyte, Inc., all rights reserved.
 #
-


This will need to be added back otherwise our file formatter in CI is going to continue failing.

brianjlai · 2022-12-13T22:03:37Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/auth/oauth.py

@@ -94,8 +96,11 @@ def get_refresh_request_body(self) -> Mapping[str, Any]:
    def get_token_expiry_date(self) -> pendulum.DateTime:
        return self._token_expiry_date

-    def set_token_expiry_date(self, value: pendulum.DateTime):
-        self._token_expiry_date = value
+    def set_token_expiry_date(self,  initial_time: pendulum.DateTime, value: Union[str, int]):


also an extra space between parameters which is throwing off the formatter

…loader

brianjlai

changes look good! thanks for incorporating the feedback in

artem1205 · 2022-12-13T22:55:38Z

/publish-cdk dry-run=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/3690043244
✅ https://github.com/airbytehq/airbyte/actions/runs/3690043244

artem1205 · 2022-12-13T23:03:18Z

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/3690089496
✅ https://github.com/airbytehq/airbyte/actions/runs/3690089496

fixed & approved

YowanR · 2022-12-14T19:19:30Z

How's this one going?

artem1205 · 2022-12-14T19:34:22Z

How's this one going?

Already Merged and closed.

artem1205 added 2 commits November 22, 2022 16:53

CDK low-code: Add token_expiry_date_format to OAuth Authenticator

b6207a8

CDK low-code: Resolve ref schema

f227983

artem1205 requested review from girarda, pedroslopez and roman-yermilov-gl November 22, 2022 16:01

artem1205 requested a review from a team as a code owner November 22, 2022 16:01

octavia-squidington-iv added the CDK Connector Development Kit label Nov 22, 2022

artem1205 added 2 commits November 22, 2022 17:42

CDK low-code: Resolve ref schema

d3657d6

Merge remote-tracking branch 'origin/artem1205/lowcode-cdk-authentica…

6b6dc3a

…tor-schemaloader' into artem1205/lowcode-cdk-authenticator-schemaloader # Conflicts: # airbyte-cdk/python/airbyte_cdk/sources/declarative/schema/json_file_schema_loader.py

artem1205 mentioned this pull request Nov 22, 2022

🎉 Source Square: migrate to Beta (YAML) #19369

Merged

37 tasks

CDK low-code: Resolve ref schema

bebf682

girarda previously requested changes Nov 22, 2022

View reviewed changes

airbyte-cdk/python/airbyte_cdk/sources/declarative/schema/json_file_schema_loader.py Outdated Show resolved Hide resolved

airbyte-cdk/python/airbyte_cdk/sources/declarative/auth/oauth.py Outdated Show resolved Hide resolved

CDK low-code: Add test for token_expiry_date_format

47874d8

artem1205 requested a review from girarda November 23, 2022 12:45

girarda requested review from brianjlai and sherifnada and removed request for pedroslopez November 28, 2022 18:15

brianjlai requested changes Nov 29, 2022

View reviewed changes

artem1205 and others added 3 commits November 29, 2022 15:47

CDK low-code: set initial time before refresh request

8d09221

Merge branch 'master' into artem1205/lowcode-cdk-authenticator-schema…

6b25720

…loader

CDK low-code: Add test schema loader

0146667

artem1205 requested a review from brianjlai November 29, 2022 19:41

CDK low-code: Add test dependencies

e7d54a8

brianjlai reviewed Nov 30, 2022

View reviewed changes

artem1205 added 3 commits November 30, 2022 14:17

CDK low-code: Refactor JsonFileSchemaLoader (inherit from original CDK)

74a2625

CDK low-code: Fix SingleUseRefreshTokenOauth2Authenticator (add token…

179a535

…_expiry_date_format)

CDK low-code: Fix tests

60827f2

artem1205 requested a review from brianjlai November 30, 2022 14:51

brianjlai requested changes Dec 1, 2022

View reviewed changes

CDK low-code: Refactor import

af966a8

lazebnyi mentioned this pull request Dec 9, 2022

Declarative schema loader $ref #19753

Closed

lazebnyi linked an issue Dec 9, 2022 that may be closed by this pull request

Declarative schema loader $ref #19753

Closed

lazebnyi mentioned this pull request Dec 9, 2022

Oauth Authenticator expiry date format #19752

Closed

lazebnyi linked an issue Dec 9, 2022 that may be closed by this pull request

Oauth Authenticator expiry date format #19752

Closed

artem1205 requested a review from brianjlai December 9, 2022 13:14

artem1205 and others added 2 commits December 10, 2022 01:01

Merge branch 'master' into artem1205/lowcode-cdk-authenticator-schema…

bc22d98

…loader

CDK low-code: Refactor JsonFileSchemaLoader

33f81e0

brianjlai reviewed Dec 13, 2022

View reviewed changes

artem1205 and others added 2 commits December 13, 2022 23:21

CDK low-code: format

523ef0b

Merge branch 'master' into artem1205/lowcode-cdk-authenticator-schema…

0a55774

…loader

artem1205 requested a review from brianjlai December 13, 2022 22:44

brianjlai approved these changes Dec 13, 2022

View reviewed changes

artem1205 merged commit cb26ce5 into master Dec 13, 2022

artem1205 deleted the artem1205/lowcode-cdk-authenticator-schemaloader branch December 13, 2022 23:11

github-actions bot mentioned this pull request Dec 15, 2022

Bump helm chart version reference to 0.43.5 #20526

Closed

octavia-squidington-iii mentioned this pull request Dec 18, 2022

Bump Airbyte version from 0.40.25 to 0.40.26 #20630

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Artem1205/lowcode cdk authenticator schemaloader #19718

Artem1205/lowcode cdk authenticator schemaloader #19718

artem1205 commented Nov 22, 2022 •

edited by lazebnyi

Loading

sherifnada commented Nov 29, 2022

brianjlai left a comment

brianjlai Nov 29, 2022

artem1205 Nov 29, 2022

brianjlai Nov 29, 2022

artem1205 Nov 29, 2022

brianjlai left a comment

brianjlai Nov 30, 2022

artem1205 Nov 30, 2022

brianjlai left a comment

brianjlai Dec 1, 2022

artem1205 Dec 1, 2022

girarda Dec 9, 2022

artem1205 Dec 10, 2022 •

edited

Loading

brianjlai Dec 12, 2022

artem1205 Dec 13, 2022

brianjlai left a comment

brianjlai Dec 13, 2022

artem1205 Dec 13, 2022

brianjlai Dec 13, 2022

artem1205 Dec 13, 2022

brianjlai left a comment

artem1205 commented Dec 13, 2022 •

edited by github-actions bot

Loading

artem1205 commented Dec 13, 2022 •

edited by github-actions bot

Loading

YowanR commented Dec 14, 2022

artem1205 commented Dec 14, 2022

		return resolved


		def resolve_ref_links(obj: Any) -> Union[Dict[str, Any], List[Any]]:

Artem1205/lowcode cdk authenticator schemaloader #19718

Artem1205/lowcode cdk authenticator schemaloader #19718

Conversation

artem1205 commented Nov 22, 2022 • edited by lazebnyi Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

sherifnada commented Nov 29, 2022

brianjlai 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

brianjlai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianjlai 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

artem1205 Dec 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianjlai 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

brianjlai left a comment

Choose a reason for hiding this comment

artem1205 commented Dec 13, 2022 • edited by github-actions bot Loading

artem1205 commented Dec 13, 2022 • edited by github-actions bot Loading

YowanR commented Dec 14, 2022

artem1205 commented Dec 14, 2022

artem1205 commented Nov 22, 2022 •

edited by lazebnyi

Loading

artem1205 Dec 10, 2022 •

edited

Loading

artem1205 commented Dec 13, 2022 •

edited by github-actions bot

Loading

artem1205 commented Dec 13, 2022 •

edited by github-actions bot

Loading