Skip to content

feat: Implement support for Incremental Table on Typeform's FormResponses table #18492

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.

Sign up for GitHub

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
kodiakhq merged 8 commits into from
Jul 8, 2024
Merged

feat: Implement support for Incremental Table on Typeform's FormResponses table #18492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
388f654
wip
marianogappa Jul 3, 2024
2561e0d
Modify Typeform plugin for incremental sync.
marianogappa Jul 5, 2024
20d3420
Set is_incremental=True
hermanschaaf Jul 5, 2024
1549b9f
Bump state client dependencies.
marianogappa Jul 8, 2024
8d7367a
Run fmt.
marianogappa Jul 8, 2024
a73edff
Fix tests.
marianogappa Jul 8, 2024
d98151b
Merge branch 'main' into mariano/state-client
marianogappa Jul 8, 2024
b6e082b
Merge branch 'main' into mariano/state-client
marianogappa Jul 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions plugins/source/typeform/plugin/plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from cloudquery.sdk import plugin
from cloudquery.sdk import schema
from cloudquery.sdk.scheduler import Scheduler, TableResolver
from cloudquery.sdk.stateclient.stateclient import StateClientBuilder

from plugin import tables
from plugin.client import Client, Spec
Expand Down Expand Up @@ -64,6 +65,9 @@ def get_tables(self, options: plugin.TableOptions) -> List[plugin.Table]:
def sync(
self, options: plugin.SyncOptions
) -> Generator[message.SyncMessage, None, None]:
state_client = StateClientBuilder.build(backend_options=options.backend_options)
self._scheduler.set_post_sync_hook(state_client.flush)

resolvers: list[TableResolver] = []
for table in self.get_tables(
plugin.TableOptions(
Expand All @@ -73,6 +77,12 @@ def sync(
)
):
resolvers.append(table.resolver)

for resolver in resolvers:
resolver.set_state_client(state_client)
for r in resolver.child_resolvers:
r.set_state_client(state_client)

return self._scheduler.sync(
self._client, resolvers, options.deterministic_cq_id
)
14 changes: 12 additions & 2 deletions plugins/source/typeform/plugin/tables/form_responses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from cloudquery.sdk.schema import Table
from cloudquery.sdk.schema.resource import Resource
from cloudquery.sdk.types import JSONType
from cloudquery.sdk.stateclient.stateclient import StateClient

from plugin.client import Client

Expand All @@ -29,11 +30,13 @@ def __init__(self) -> None:
Column("variables", JSONType()),
Column("tags", JSONType()),
],
is_incremental=True,
)
self._resolver = FormResponsesResolver(table=self)

@property
def resolver(self):
return FormResponsesResolver(table=self)
return self._resolver
Copy link
Copy Markdown
Contributor Author

@marianogappa marianogappa Jul 8, 2024

Choose a reason for hiding this comment

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

This is a dangerous anti-pattern we should be aware of. It was creating a brand new resolver every time the property was being referenced (note that by adding @property, you don't call resolver(), but just resolver).

Copy link
Copy Markdown

@dkeyer-twilio dkeyer-twilio Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@marianogappa What's the risk of this usage, out of curiosity? Is there a performance hit?

Copy link
Copy Markdown
Contributor Author

@marianogappa marianogappa Jul 11, 2024

Choose a reason for hiding this comment

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

@dkeyer-twilio oh sorry, I should have been more clear. There's no performance issue.

  • resolver here is acting as a getter, and one doesn't expect a getter to create a brand new object.
  • Objects are stateful. One wouldn't normally expect to set some state on an object and then not have it later.
  • In this case, the .sync() method calls resolver.set_state_client(...), and this method would erase that state.

I think this pattern causes very unintuitive behaviour. I certainly lost an hour on it.



class FormResponsesResolver(TableResolver):
Expand All @@ -43,8 +46,15 @@ def __init__(self, table) -> None:
def resolve(
self, client: Client, parent_resource: Resource
) -> Generator[Any, None, None]:
since = self.state_client.get_key("typeform_form_responses_since")

for form_response in client.client.list_form_responses(
form_id=parent_resource.item["id"]
form_id=parent_resource.item["id"],
since=since,
):
if not since or form_response["submitted_at"] >= since:
since = form_response["submitted_at"]
self.state_client.set_key("typeform_form_responses_since", since)

form_response["form_id"] = parent_resource.item["id"]
yield form_response
3 changes: 2 additions & 1 deletion plugins/source/typeform/plugin/tables/forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,11 @@ def __init__(self) -> None:
],
relations=[FormResponses()],
)
self._resolver = FormsResolver(table=self)

@property
def resolver(self):
return FormsResolver(table=self)
return self._resolver


class FormsResolver(TableResolver):
Expand Down
8 changes: 5 additions & 3 deletions plugins/source/typeform/plugin/typeform/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ def list_forms(self, page=1):
if resp["page_count"] > page:
yield from self.list_forms(page + 1)

def list_form_responses(self, form_id, page=1):
params = {"page": page, "page_size": 1000}
def list_form_responses(self, *, form_id, since, page=1):
params = {"page": page, "page_size": 1000, "since": since}
resp = self._get(f"/forms/{form_id}/responses", params=params)
if resp.status_code != 200:
raise Exception(f"Failed to list form responses: {resp.text}")
Expand All @@ -35,4 +35,6 @@ def list_form_responses(self, form_id, page=1):
yield form

if resp["page_count"] > page:
yield from self.list_form_responses(form_id, page + 1)
yield from self.list_form_responses(
form_id=form_id, since=since, page=page + 1
)
4 changes: 2 additions & 2 deletions plugins/source/typeform/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
cloudquery-plugin-sdk==0.1.26
cloudquery-plugin-sdk==0.1.27
pyarrow==15.0.2
requests==2.32.3
pytest==8.2.1
pytest==8.2.2
pandas==2.2.2
3 changes: 2 additions & 1 deletion plugins/source/typeform/tests/tables/test_forms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ def test_forms(mock_typeform_client):
]
client.list_form_responses.return_value = [
{
"response_id": "response_id",
"answers": [],
"submitted_at": "2017-09-14T22:38:22Z",
},
]

Expand Down
2 changes: 1 addition & 1 deletion plugins/source/typeform/tests/typeform/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ def test_list_forms_responses(self):
base_url="http://localhost:{}".format(self.mock_server_port),
access_token="fake",
)
forms = list(client.list_form_responses("form1"))
forms = list(client.list_form_responses(form_id="form1", since=None))
assert len(forms) == 2
assert forms[0]["submitted_at"] == "2017-09-14T22:38:22Z"
assert forms[1]["submitted_at"] == "2017-09-14T22:33:56Z"