[16.0][FIX] connector_sapb1: enforce SAP Street length in the partner adapter - #939
Open
eantones wants to merge 9 commits into
Open
[16.0][FIX] connector_sapb1: enforce SAP Street length in the partner adapter#939eantones wants to merge 9 commits into
eantones wants to merge 9 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 16.0 #939 +/- ##
==========================================
- Coverage 47.35% 47.21% -0.15%
==========================================
Files 335 335
Lines 6799 6830 +31
Branches 1034 1042 +8
==========================================
+ Hits 3220 3225 +5
- Misses 3544 3570 +26
Partials 35 35 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This reverts commit ef59229.
eantones
force-pushed
the
16.0-fix-connector_sapb1-street_length_adapter
branch
3 times, most recently
from
July 2, 2026 07:36
9107a1c to
2f8da6d
Compare
_convert_format() applies a per-field converter mapping to record values, but there was no equivalent for search domains: a model adapter that needs its domain values normalized the same way as its written values had nowhere to do it without walking the domain itself. Add _convert_format_domain_values(), the domain-side counterpart of _convert_format(): it applies the converter mapping per clause by field name, element-wise on list/tuple values, and nothing else. Backend type formatting stays in _convert_format_domain(), one concern per function.
_format_partner_domain() passed its converter mapping as a second positional argument to _convert_format_domain(), which accepts none: any execution raised "TypeError: _convert_format_domain() takes 2 positional arguments but 3 were given". The bug never surfaced because its only consumer, the alternate-key matching of partner addresses, aborted before reaching it (see the Block removal). Compose the two base helpers instead: format the domain types first, then apply the partner converters with _convert_format_domain_values().
eantones
force-pushed
the
16.0-fix-connector_sapb1-street_length_adapter
branch
3 times, most recently
from
July 2, 2026 08:49
b7f310e to
83ad31c
Compare
_get_partner() performs the request, parses the JSON and validates the error cases, but falls off the end without returning the parsed result, so every consumer that needs the payload dereferences None and crashes with "'NoneType' object is not subscriptable". The alternate-key matching of partner addresses reads the partner's BPAddresses from this method: without the return, the first lookup crashes as soon as the matching runs (see the Block removal). Return the parsed partner, as the other _get_* methods do.
The Block address field was referenced in three places but never used: * no export mapping produces it, so exported addresses never carry it; * the /Block value converter (create) and the Block domain converter (search) in the partner adapter therefore never fire; * the partner binder included Block in external_alt_id, the alternate match key. Keeping Block in external_alt_id was not harmless. The alternate-key lookup resolves the key from the export mapper output, and a component missing from that output makes the resolved key null, which aborts the lookup (is_id_null guard). As the mapper never produces Block, the alternate-key matching of partner addresses never executed: every unbound partner went straight to create. This is observable in a production database as duplicated identical address rows: 496 of 7,515 address rows (6.6%) across the 19 mapped partner accounts. Measured on that same dataset, the number of distinct match keys is identical with and without Block (7,019 = 7,019): Block adds zero discrimination, so dropping it cannot alter any match outcome. The only rows with Block populated (203, all entered manually in SAP) are already distinct by the remaining key fields. Besides removing dead entries, this re-enables the intended alternate-key matching for partners whose key is complete: repeat addresses now link to the existing backend row instead of duplicating it. Where identical duplicates already exist in the backend, the lookup now raises "the alternate external id is not unique" and the job fails visibly instead of silently adding another duplicate. Note: if a deployment extends the partner export mapper to produce Block, dropping it from external_alt_id changes that deployment's match key; no in-tree module does.
SAP B1 stores business partner address streets in CRD1.Street, an NVARCHAR(100) column; exporting a longer value makes the backend reject the whole document with "Value too long for column 'Street'". Odoo has no such limit, and the mapper deliberately concatenates street and street2 into Street (the Block field does not reach printed documents), so overflow is possible on any long address. Apply the 100-character cap in the partner adapter on every path that carries a Street value to or against the backend: * _format_partner_values (create), so written values always fit; * _format_partner_domain (search), so alternate-key lookups compare exactly what create writes; otherwise an over-long street would never match its stored, truncated row and every repeat order would create a duplicate address; * write(), which previously bypassed _format_partner_values entirely, now normalizes values like create() does, so updates get the same falsy-to-None normalization and length cap. Truncation is lossy for streets over 100 characters by design: a slightly clipped address that reaches the backend beats a document that cannot be exported at all. All 51,806 existing CRD1 address rows in the reference production schema are already <= 100 characters, so the cap changes nothing for previously synced data.
…lues The partner address lookup compared its two sides with different conventions: the lookup domain converts empty values to None (_format_partner_domain), while the rows fetched from the backend kept empty strings in empty components. The comparison is strict, so an address with any empty component could never match its stored counterpart. The defect never surfaced because the only consumer of this comparison, the alternate-key matching of partner addresses, never ran (see the Block removal). Apply the same value converters used for writing (_format_partner_values) to each fetched row, so both sides of the comparison follow one convention.
eantones
force-pushed
the
16.0-fix-connector_sapb1-street_length_adapter
branch
from
July 2, 2026 10:12
83ad31c to
fcda448
Compare
…ookup An ID cannot contain nulls by definition: a null component means the record has no identity, and that is exactly what is_id_null() checks. An alternate key is a different kind of identifier: a natural key built from business data, where a null component can be a legitimate part of the identity (an address without zip stores NULL, and an identical existing address is found by matching on that NULL). The lookup applied the ID contract to the alternate key, aborting on any null component, so records with legitimately empty fields were never matched and were re-created instead. Give the alternate key its own contract: external_alt_id_nullable_fields declares, per component, where null is a value (nullable) and where it is absence of identity (mandatory, the default). _check_external_alt_id() replaces the is_id_null() call and enforces it: * a null on a nullable component participates in the lookup as a value to match on; * a null on a mandatory component raises InvalidDataError: the record can neither be matched now nor ever be matched later, so creating it would plant a permanently unmatchable row (a future duplicate); * a component missing from the mapper output raises ValidationError: a binder/mapper configuration error that silently disables matching for the whole model, exactly how an unmapped key field went unnoticed for years; * declared nullable fields must belong to the key and at least one component must stay mandatory, so a single-component alternate id is always mandatory. In-tree export mappers already validate the mandatory fields themselves, so these checks act as a safety net with precise messages rather than a behavior change for healthy data. The old TODO asking to move this block into a hook is fulfilled by the dedicated overridable method and removed. A nullable null travels through the lookup domain as a None value, so _convert_format_domain() now passes None through, exactly as _convert_format() already does for record values; it had never received one because the old lookup aborted before building a domain.
CardCode and AddressName2 identify the account and the addressee and remain mandatory: a lookup without them would be meaningless. AddressName3, Street, ZipCode and City describe the address and are declared nullable: an address without email, street, zip or city stores NULL in those columns, and an identical existing address must match on those NULLs instead of being re-created.
eantones
force-pushed
the
16.0-fix-connector_sapb1-street_length_adapter
branch
from
July 2, 2026 11:21
fcda448 to
c6f7eee
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SAP B1 rejects a whole document export with
Value too long for column 'Street'when the concatenatedstreet+street2exceedsCRD1.Street(NVARCHAR(100)). #937 capped the value in the export mapper; this PR moves that concern to its proper layer, removes a dead field, and makes the partner address matching this uncovers actually work. Nine commits, each incremental:_convert_format_domain_values(), the domain-side counterpart of_convert_format(): it applies a per-field converter mapping per clause (element-wise oninlists) and nothing else. Backend type formatting stays in_convert_format_domain()— one concern per function._format_partner_domain()passed its converter mapping as a second positional argument to_convert_format_domain(), which accepts none: any execution raisedTypeError(latent, unreachable until now — see below). Compose the two base helpers instead: format the domain types, then apply the partner converters._get_partner()performed the request and parsed the response but fell off the end without returning it — every consumer needing the payload dereferencesNone. Latent for the same reason as the other two fixes; the revived matching reads the partner's addresses from it. Return the parsed partner.Blockfrom the partner adapter converters and the binderexternal_alt_id. The export mapper never producesBlock, so the alternate-key resolution always contained a null component and aborted: partner alternate-key matching has never executed — every unbound partner address went straight to create. Measured on a production dataset (19 mapped partner accounts, 7,515 address rows): distinct match keys with vs. withoutBlockare identical (7,019 = 7,019), soBlockadds zero discrimination; 6.6% of rows (496) are duplicates accumulated precisely because matching never ran.write(), which previously bypassed value normalization entirely.search_readwith the same value converters used for writing. The lookup domain converts empty values to None while fetched rows kept empty strings, so the two sides of the comparison followed different conventions and an address with any empty component could never match — latent for the same reason as theTypeErrorabove.external_alt_id_nullable_fields) — a null on a declared-nullable component is a legitimate identity value (an address without zip stores NULL, so NULL is what an identical existing record contains); a null on a mandatory component raisesInvalidDataError(a record with incomplete identity can neither be matched now nor ever be matched later — silently creating it would plant a permanently unmatchable row; the all-null key is the extreme case of the same rule); a component missing from the mapper output raisesValidationError(binder/mapper configuration error that silently disables matching for the whole model — exactly how theBlockissue stayed invisible for years). At least one component must remain mandatory and nullable declarations must belong to the key, both enforced.AddressName3,Street,ZipCode,Citynullable;CardCodeandAddressName2stay mandatory.Review notes
Alternate-key matching for partner addresses becomes live for the first time: repeat addresses now link to the existing backend row instead of duplicating it, including addresses with empty nullable fields.
Where identical duplicates already exist in the backend (the 496 legacy rows), the lookup raises
"the alternate external id ... is not unique"and the job fails visibly — consistent with how every other backend anomaly surfaces (e.g. a locked posting period). A one-off backend dedup of those legacy rows ends these failures permanently; the exact row list can be extracted on request.Since this path has never run in production, a functional pass on a test database before deploying is recommended.
If a deployment extends the partner export mapper to produce
Block, dropping it fromexternal_alt_idchanges that deployment's match key; no in-tree module does.Deployment dependency: this PR must run together with [16.0][FIX] connector_sapb1: enable allow_commit on export_record job funct… #895 (already aggregated in production via
refs/pull/895/head): the revived matching path persists bindings throughbind_export(), whose eager commit needs [16.0][FIX] connector_sapb1: enable allow_commit on export_record job funct… #895'sallow_commitjob functions plus its cursor-rebind of job args. Without [16.0][FIX] connector_sapb1: enable allow_commit on export_record job funct… #895, every match ends in "Commit is forbidden in queue jobs".Test plan