added dynamic schema #1
Conversation
Co-authored-by: Copilot <[email protected]>
…into dynamic_schema_formkeep
tap_formkeep/schema.py
Outdated
| return ["null", "number"] | ||
|
|
||
| if isinstance(value, list): | ||
| return ["null", "array"] |
There was a problem hiding this comment.
Need unit test for this. It should also handle nested fields.
tap_formkeep/client.py
Outdated
|
|
||
| LOGGER = get_logger() | ||
| REQUEST_TIMEOUT = 300 | ||
| DEFAULT_USER_AGENT = 'Singer.io Bing Ads Tap' |
There was a problem hiding this comment.
User-Agent references 'Bing Ads Tap' instead of 'Formkeep Tap'. This should be corrected to accurately identify this tap.
| DEFAULT_USER_AGENT = 'Singer.io Bing Ads Tap' | |
| DEFAULT_USER_AGENT = 'Singer.io Formkeep Tap' |
| return {"type": ["null", "number"]} | ||
|
|
||
| if isinstance(value, str): | ||
| if DATETIME_REGEX.match(value) or DATE_REGEX.match(value): |
There was a problem hiding this comment.
The DATETIME_REGEX pattern allows space as a date-time separator (pattern includes [T ]), but this does not produce RFC 3339 compliant datetime strings. Per coding guideline #1000000 section 3, only RFC 3339 format with 'T' separator should be accepted. Remove the space alternative from the regex.
| raw_ids = config.get("form_ids", []) | ||
|
|
||
| if isinstance(raw_ids, str): | ||
| form_ids = ast.literal_eval(raw_ids) |
There was a problem hiding this comment.
Using ast.literal_eval() on user configuration input without error handling is risky. If the string is malformed, it will raise a ValueError. Add try-except to handle parsing errors gracefully and log a meaningful error message.
| form_ids = ast.literal_eval(raw_ids) | |
| try: | |
| form_ids = ast.literal_eval(raw_ids) | |
| except (ValueError, SyntaxError) as e: | |
| LOGGER.error(f"Failed to parse 'form_ids' from config: {raw_ids!r}. Error: {e}") | |
| form_ids = [] |
|
|
||
| DATE_REGEX = re.compile(r"^\d{4}-\d{2}-\d{2}$") | ||
| TIME_REGEX = re.compile(r"^\d{2}:\d{2}(:\d{2})?$") |
There was a problem hiding this comment.
[nitpick] Module-level regex patterns should be named with uppercase convention (already correct) but consider adding docstrings explaining their purpose and the formats they match to improve code maintainability.
| DATE_REGEX = re.compile(r"^\d{4}-\d{2}-\d{2}$") | |
| TIME_REGEX = re.compile(r"^\d{2}:\d{2}(:\d{2})?$") | |
| # Matches dates in the format YYYY-MM-DD (e.g., 2024-06-01). | |
| DATE_REGEX = re.compile(r"^\d{4}-\d{2}-\d{2}$") | |
| # Matches times in the format HH:MM or HH:MM:SS (e.g., 14:30 or 14:30:59). | |
| TIME_REGEX = re.compile(r"^\d{2}:\d{2}(:\d{2})?$") | |
| # Matches datetimes in the format YYYY-MM-DDTHH:MM:SSZ, with optional timezone offsets or ' UTC'. | |
| # Examples: 2024-06-01T14:30:59Z, 2024-06-01 14:30:59+02:00, 2024-06-01T14:30:59 UTC |
| if not submissions: | ||
| continue | ||
|
|
||
| first_submission = submissions[0] |
There was a problem hiding this comment.
Inferring schema from only the first submission is unreliable. If the first submission has null values or missing fields, the schema will be incomplete. Consider iterating through multiple submissions and merging their schemas, or at minimum, document this limitation.
* initial push for sync changes * Update tap_formkeep/streams/abstracts.py Co-authored-by: Copilot <[email protected]> * remove spam from params * integration tests * discovery test * integration tests * change logic for get records * change page size * user agent update * default page size * remove unwanted import * update camel case --------- Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Observed field value like these "description of what you'd like to discuss": "[email protected] description" where there are spaces in between field keys. We should replace those spaces by _.
Description of change
This PR introduces the implementation of dynamic schema for formkeep.
(SAC-29462)
Manual QA steps
Risks
Rollback steps