-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Source Google Analytics Data API: ability to add multiple property ids #30152
Changes from all commits
af49ca9
c462328
cda8df1
a3a4f1f
260d51a
815f1ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
# | ||
# Copyright (c) 2023 Airbyte, Inc., all rights reserved. | ||
# | ||
|
||
|
||
import logging | ||
from typing import Any, List, Mapping | ||
|
||
from airbyte_cdk.config_observation import create_connector_config_control_message | ||
from airbyte_cdk.entrypoint import AirbyteEntrypoint | ||
from airbyte_cdk.sources import Source | ||
from airbyte_cdk.sources.message import InMemoryMessageRepository, MessageRepository | ||
|
||
logger = logging.getLogger("airbyte_logger") | ||
|
||
|
||
class MigrateCustomReports: | ||
""" | ||
This class stands for migrating the config at runtime, | ||
while providing the backward compatibility when falling back to the previous source version. | ||
|
||
Specifically, starting from `1.3.0`, the `property_id` property should be like : | ||
> List(["<property_id 1>", "<property_id 2>", ..., "<property_id n>"]) | ||
instead of, in `1.2.0`: | ||
> JSON STR: "<property_id>" | ||
""" | ||
|
||
message_repository: MessageRepository = InMemoryMessageRepository() | ||
migrate_from_key: str = "property_id" | ||
migrate_to_key: str = "property_ids" | ||
|
||
@classmethod | ||
def should_migrate(cls, config: Mapping[str, Any]) -> bool: | ||
""" | ||
This method determines whether config require migration. | ||
Returns: | ||
> True, if the transformation is neccessary | ||
> False, otherwise. | ||
""" | ||
if cls.migrate_from_key in config: | ||
return True | ||
return False | ||
|
||
@classmethod | ||
def transform_to_array(cls, config: Mapping[str, Any], source: Source = None) -> Mapping[str, Any]: | ||
# assign old values to new property that will be used within the new version | ||
config[cls.migrate_to_key] = config[cls.migrate_to_key] if cls.migrate_to_key in config else [] | ||
data = config.pop(cls.migrate_from_key) | ||
if data not in config[cls.migrate_to_key]: | ||
config[cls.migrate_to_key].append(data) | ||
return config | ||
|
||
@classmethod | ||
def modify_and_save(cls, config_path: str, source: Source, config: Mapping[str, Any]) -> Mapping[str, Any]: | ||
# modify the config | ||
migrated_config = cls.transform_to_array(config, source) | ||
# save the config | ||
source.write_config(migrated_config, config_path) | ||
# return modified config | ||
return migrated_config | ||
|
||
@classmethod | ||
def emit_control_message(cls, migrated_config: Mapping[str, Any]) -> None: | ||
# add the Airbyte Control Message to message repo | ||
cls.message_repository.emit_message(create_connector_config_control_message(migrated_config)) | ||
# emit the Airbyte Control Message from message queue to stdout | ||
for message in cls.message_repository._message_queue: | ||
print(message.json(exclude_unset=True)) | ||
|
||
@classmethod | ||
def migrate(cls, args: List[str], source: Source) -> None: | ||
""" | ||
This method checks the input args, should the config be migrated, | ||
transform if neccessary and emit the CONTROL message. | ||
""" | ||
# get config path | ||
config_path = AirbyteEntrypoint(source).extract_config(args) | ||
# proceed only if `--config` arg is provided | ||
if config_path: | ||
# read the existing config | ||
config = source.read_config(config_path) | ||
# migration check | ||
if cls.should_migrate(config): | ||
cls.emit_control_message( | ||
cls.modify_and_save(config_path, source, config), | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
"$schema": "https://json-schema.org/draft-07/schema#", | ||
"title": "Google Analytics (Data API) Spec", | ||
"type": "object", | ||
"required": ["property_id", "date_ranges_start_date"], | ||
"required": ["property_ids", "date_ranges_start_date"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you test those changes manually for backward compatibility? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did, but it seems I missed something. Now migration is prepared and everithing is tested |
||
"additionalProperties": true, | ||
"properties": { | ||
"credentials": { | ||
|
@@ -76,14 +76,16 @@ | |
} | ||
] | ||
}, | ||
"property_id": { | ||
"type": "string", | ||
"title": "Property ID", | ||
"property_ids": { | ||
"title": "A list of your Property IDs", | ||
"description": "The Property ID is a unique number assigned to each property in Google Analytics, found in your GA4 property URL. This ID allows the connector to track the specific events associated with your property. Refer to the <a href='https://developers.google.com/analytics/devguides/reporting/data/v1/property-id#what_is_my_property_id'>Google Analytics documentation</a> to locate your property ID.", | ||
"pattern": "^[0-9]*$", | ||
"pattern_descriptor": "123...", | ||
"examples": ["1738294", "5729978930"], | ||
"order": 1 | ||
"order": 1, | ||
"type": "array", | ||
"items": { | ||
"type": "string", | ||
"pattern": "^[0-9]*$" | ||
}, | ||
"examples": [["1738294", "5729978930"]] | ||
}, | ||
"date_ranges_start_date": { | ||
"type": "string", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
from unittest.mock import patch | ||
|
||
from airbyte_cdk.entrypoint import AirbyteEntrypoint | ||
from source_google_analytics_data_api import SourceGoogleAnalyticsDataApi | ||
from source_google_analytics_data_api.config_migrations import MigrateCustomReports | ||
|
||
|
||
@patch.object(SourceGoogleAnalyticsDataApi, "read_config") | ||
@patch.object(SourceGoogleAnalyticsDataApi, "write_config") | ||
@patch.object(AirbyteEntrypoint, "extract_config") | ||
def test_migration(ab_entrypoint_extract_config_mock, source_write_config_mock, source_read_config_mock): | ||
source = SourceGoogleAnalyticsDataApi() | ||
|
||
source_read_config_mock.return_value = { | ||
"credentials": { | ||
"auth_type": "Service", | ||
"credentials_json": "<credentials string ...>" | ||
}, | ||
"custom_reports": "<custom reports out of current test>", | ||
"date_ranges_start_date": "2023-09-01", | ||
"window_in_days": 30, | ||
"property_id": "111111111" | ||
} | ||
ab_entrypoint_extract_config_mock.return_value = "/path/to/config.json" | ||
|
||
def check_migrated_value(new_config, path): | ||
assert path == "/path/to/config.json" | ||
assert "property_id" not in new_config | ||
assert "property_ids" in new_config | ||
assert "111111111" in new_config["property_ids"] | ||
assert len(new_config["property_ids"]) == 1 | ||
|
||
source_write_config_mock.side_effect = check_migrated_value | ||
|
||
MigrateCustomReports.migrate(["--config", "/path/to/config.json"], source) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,7 +33,7 @@ | |
def patch_base_class(): | ||
return { | ||
"config": { | ||
"property_id": "108176369", | ||
"property_ids": ["108176369"], | ||
"credentials": {"auth_type": "Service", "credentials_json": json_credentials}, | ||
"date_ranges_start_date": datetime.datetime.strftime((datetime.datetime.now() - datetime.timedelta(days=1)), "%Y-%m-%d"), | ||
} | ||
|
@@ -43,7 +43,7 @@ def patch_base_class(): | |
@pytest.fixture | ||
def config(): | ||
return { | ||
"property_id": "108176369", | ||
"property_ids": ["108176369"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add unit test with old config and try test backward compatibility in that way There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in scope of migration |
||
"credentials": {"auth_type": "Service", "credentials_json": json_credentials}, | ||
"date_ranges_start_date": datetime.datetime.strftime((datetime.datetime.now() - datetime.timedelta(days=1)), "%Y-%m-%d"), | ||
"custom_reports": json.dumps([{ | ||
|
@@ -117,7 +117,7 @@ def test_check(requests_mock, config_gen, config_values, is_successful, message) | |
assert source.check(logger, config_gen(**config_values)) == AirbyteConnectionStatus(status=is_successful, message=message) | ||
if not is_successful: | ||
with pytest.raises(AirbyteTracedException) as e: | ||
source.check(logger, config_gen(property_id="UA-11111111")) | ||
source.check(logger, config_gen(property_ids=["UA-11111111"])) | ||
assert e.value.failure_type == FailureType.config_error | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,6 +39,7 @@ def patch_base_class(mocker): | |
|
||
return { | ||
"config": { | ||
"property_ids": ["496180525"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if an old config has not property_ids what happens next? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Figured out in scope of migration |
||
"property_id": "496180525", | ||
"credentials": {"auth_type": "Service", "credentials_json": json_credentials}, | ||
"dimensions": ["date", "deviceCategory", "operatingSystem", "browser"], | ||
|
@@ -307,6 +308,7 @@ def test_stream_slices(): | |
|
||
def test_read_incremental(requests_mock): | ||
config = { | ||
"property_ids": [123], | ||
"property_id": 123, | ||
"date_ranges_start_date": datetime.date(2022, 12, 29), | ||
"window_in_days": 1, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add spaces between condition blocks and add comments for them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done