fix: Require batch encoding for BATCH output - #3779
edgarrmondragon wants to merge 6 commits into
Conversation
…er BATCH output is opted into Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
|
@sourcery-ai title |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe PR tightens batch output configuration validation by requiring an encoding format, preventing BATCH outputs from being configured without specifying how files should be encoded. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
batch_config.encoding.format setting required whenever BATCH output is opted into
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3779 +/- ##
=======================================
Coverage 94.47% 94.47%
=======================================
Files 74 74
Lines 6298 6303 +5
Branches 771 771
=======================================
+ Hits 5950 5955 +5
Misses 260 260
Partials 88 88
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="singer_sdk/helpers/capabilities.py" line_range="206-209" />
<code_context>
description="Compression format to use for batch files.",
),
),
+ required=True,
),
Property(
</code_context>
<issue_to_address>
**issue (bug_risk):** The new `required=True` applies to the `encoding` property of `batch_config`, not to the nested `format` property. A BATCH configuration containing `"encoding": {}` still passes the generated JSON Schema, so the required batch encoding format is not enforced.
**Triggers:** When BATCH output is enabled and the configuration includes `batch_config.encoding` without `format`.
**Suggested fix:** Set `required=True` on the nested `Property("format", ...)` instead of on the `encoding` property.
```suggestion
Property(
"format",
StringType,
title="Batch Encoding Format",
description="Format to use for batch files.",
required=True,
),
Property(
"compression",
StringType,
allowed_values=["gzip", "none"],
title="Batch Compression Format",
description="Compression format to use for batch files.",
),
),
```
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: singer_sdk/helpers/capabilities.py:209
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="singer_sdk/helpers/capabilities.py" line_range="201" />
<code_context>
title="Batch Encoding Format",
description="Format to use for batch files.",
+ required=True,
),
Property(
"compression",
</code_context>
<issue_to_address>
**issue (bug_risk):** Making `format` required only inside the optional `encoding` object does not require it for every `batch_config`. A configuration such as `{"batch_config": {"storage": {"root": "..."}}}` still passes the generated schema because `encoding` itself is optional, then `BatchConfig.from_dict()` raises `TypeError` when BATCH processing constructs `BatchConfig` without an `encoding`.
**Triggers:** When BATCH output is enabled with `batch_config` containing storage or other settings but no `encoding` object.
**Suggested fix:** Make `encoding` required whenever `batch_config` is present, or add a conditional schema requiring `encoding.format` for BATCH configurations.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: singer_sdk/helpers/capabilities.py:201
Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
Sourcery withdrew this approval because the latest commits introduced blocking findings.
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="singer_sdk/helpers/capabilities.py" line_range="209" />
<code_context>
description="Compression format to use for batch files.",
),
),
+ required=True,
),
Property(
</code_context>
<issue_to_address>
**issue (bug_risk):** The schema requires the `encoding` object but leaves its `format` property optional, so a BATCH configuration such as `{"encoding": {"compression": "gzip"}}` passes validation and later `BaseBatchFileEncoding.from_dict` raises `TypeError` because the required `format` constructor argument is missing.
**Triggers:** When BATCH output is enabled and the configuration supplies encoding without a format.
**Suggested fix:** Set `required=True` on the nested `format` Property, rather than only on the `encoding` Property.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: singer_sdk/helpers/capabilities.py:209
Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
Summary by Sourcery
Require batch-capable taps to configure the encoding format for BATCH output while keeping it optional for targets.
Bug Fixes:
Enhancements: