Performance and usability enhancements, bug fixes, and CI/CD automation for Shuffle - #22
Open
imranakram wants to merge 46 commits into
Open
imranakram wants to merge 46 commits into
imranakram wants to merge 46 commits into
Conversation
- Add ExecuteMultipleRequest batching for import (configurable BatchSize, UI field, schema support) - Support Multi-Select OptionSet (OptionSetValueCollection) in export/import - Ensure deterministic XML export ordering (alphabetical attributes) - Optimize attribute filtering and deduplication for performance - Hoist metadata lookups out of inner loops - Fix CSV/text export off-by-one error and improve update error logging - Add GitHub Actions for CI build and release automation - Update solution, README, and nuspecs for new features and copyright
…ogs. Updated ShuffleDataImport.cs and ShuffleSolutionImport.cs to use formatted values where available, improving clarity during data matching and solution import operations. Also adjusted logging in Shuffler.cs to avoid double-formatting messages.
…s available natively
Expanded README to include detailed documentation for Shuffle Builder, Runner, and Deployer tools, including features, usage, and XrmToolBox availability. Added a comprehensive schema reference for Shuffle Definition XML with tables for all elements and attributes. Clarified terminology and reorganized recent changes and bug fix sections for improved clarity and usability.
imranakram
added
documentation
Owner
|
Before I read the details: I recommend that we use |
High-performance bulk import now uses CreateMultiple/UpdateMultiple on Dataverse (online), with automatic detection and fallback to ExecuteMultipleRequest for on-premises or unsupported entities. Batch size default lowered to 100 per Microsoft's guidance. Batch logic refactored for robust fallback and detailed logging. Updated docs and solution items accordingly. No breaking changes to public API or config.
Introduce DeferStateAndOwner option to enable two-pass import: statecode, statuscode, and ownerid are stripped for bulk import, then applied in a second pass using bulk operations. This significantly improves performance (3-5×) for datasets with state/owner attributes. Includes README documentation, new struct definitions, and robust error handling. Feature is opt-in and backwards compatible.
Automatically uses UpsertMultiple for imports with Save="CreateUpdate" and CreateWithId="true" on Dataverse, eliminating pre-retrieval queries and significantly improving performance. Adds batching and fallback logic for all CRM versions. Updates documentation to explain UpsertMultiple optimization, usage, and compatibility. No breaking changes; full backward compatibility maintained.
Added a detailed "Import Path Selection" section to the README, explaining how Shuffle chooses between Upsert and Match-based import strategies based on configuration. Clarified when PreRetrieveAll is used or bypassed, and included a summary table for quick reference. Improved documentation for PreRetrieveAll to specify its relevance to each path, helping users optimize import performance.
Collaborator
Author
|
I have added support for CreateMultiple etc, please have a look and test as well if you can @rappen |
Previously, Upsert was enabled for CreateUpdate with IDs, match attributes, and no delete. Now, it also requires UpdateIdentical=true, ensuring Upsert is only used when identical records can be updated, since Upsert cannot skip identical records.
The Latebound Constants Generator settings pointed at an absolute path on one developer's machine (C:\Dev\GitHub\Shuffle\...\Rappen.XTB.Shuffle), which no longer exists and never existed for anyone else. Point it at the actual location of the generated Const.cs instead, so regenerating constants works from a fresh clone. The namespace is deliberately left as Cinteros.Crm.Utils.Shuffle - that is still what Const.cs and the rest of Xrm.Shuffle.Core declare.
ExecuteMultipleSettings.ContinueOnError is set to !StopOnError. With
StopOnError="true" the platform stops at the first fault and returns no
response items for the requests after it, so the per-item loop found no
response for those indexes. Because the loop tested responseItem?.Fault !=
null, a missing response fell into the success branch: the record was counted
as created/updated/deleted, logged as such, added to the returned references
and, for creates, fed to MapGuid with Guid.Empty - poisoning the guid map for
later blocks. The run then reported success while records were silently
missing from the target.
Handle the three cases separately in all four ExecuteMultiple loops (create,
update, upsert, delete):
- no response -> the request was never executed; count it as failed and log
"Not Executed" so the row shows up in the import log
- fault -> count as failed and log, as before; when StopOnError is set,
log how many records in the batch were not executed and
abort the run, which is how the tool behaved before batching
was introduced
- response -> success
Also narrow the try/catch so it wraps only Service.Execute. It previously
covered response processing as well, so any exception raised while reading
responses re-ran the whole batch through the sequential fallback, re-applying
records the server had already committed. For the same reason the sequential
fallback is now skipped when StopOnError is set - the other batch paths
(CreateMultiple, UpdateMultiple, UpsertMultiple) already rethrow there.
The XSD advertised default="200" but XmlSerializer takes the value from the generated class constructor, which sets 100 (Resources/ShuffleDefinition.cs:111). Every definition that does not set BatchSize therefore got 100, not the 200 the schema and the release notes promised. Align the schema on 100 rather than raising the constructor to 200, so that merging this branch does not silently double the batch size for existing definitions.
7.3.0 carries a known vulnerability.
Two problems with the pre-flush that guarded the match path. It was in the wrong place to protect intra-block lookups. ReplaceGuids runs at the top of every iteration, rewriting the record's lookups from guidmap, and it runs before the match path is reached - so a record pointing at another record still waiting in the create batch kept the source-system id, because the flush that would have mapped it happened one step too late. Flush before ReplaceGuids instead, and only when the record actually references something pending, which is what ReferencesPendingCreate now checks. It also fired unconditionally, which defeated batching for every matched block. That is every data block in a definition written before batching existed. Under PreRetrieveAll the flush buys nothing anyway: cAllRecordsToMatch is a snapshot taken once at the start of the block and never appended to, so it cannot see records created during the block whether the batch is flushed or not. Restrict the flush to live match queries, which do have to see what has been created so far. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DeferStateAndOwner strips statecode, statuscode and ownerid off a record so that it stays batchable, and replays them once the block is done. The replay needs the record's actual id, which UpdateDeferredActualIds filled in from the import loop - under !newid.Equals(Guid.Empty). newid is only assigned on the paths that create or update a record inline. A record handed to the create batch leaves the iteration with newid still Guid.Empty, because at that point it has no id yet: the id arrives when the batch is sent, inside the flush methods. So every deferred change for a batched create kept ActualId empty and its state or owner was silently never applied. Record the id where it becomes known instead. The four create flush methods now call RecordCreatedId, which maps the guid as before and also updates any deferred change waiting for that record. Deliberately not folded into MapGuid: the guid map skips ids that are unchanged or already mapped, and a CreateWithId record - whose old and new id are equal - is exactly the case where the deferred change still needs its id. The update and upsert paths already set newid and are unchanged. The two deferred lists become fields so the flush methods can reach them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The attribute is read by the import (ShuffleDataImport.cs) and exists on the generated DataBlockImport class, but was never declared in the schema. Since ValidateDefinitionXml runs from the ShuffleDefinition setter on every run, any definition setting the attribute failed validation before the import started - so the feature could not be reached at all. Optional and defaulting to false, matching the constructor, so existing definitions are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Import node gained the DeferStateAndOwner attribute, but the builder had no control for it, so a definition could only get it by hand-editing the XML. Adds a checkbox bound to the attribute, plus a "?" marker and a tooltip explaining what deferring buys: records carrying statecode, statuscode or ownerid are not batchable, so stripping those attributes and applying them in a second pass keeps the records themselves on the batched path. Also corrects the batch size default carried in the control tag from 200 to 100, which is what the schema and the generated definition class actually use.
- BatchSize default is 100, not 200, in both the nuspecs and the README table. - The Upsert path also requires UpdateIdentical=true; the condition list and the path-selection table were missing it. - Replaced the unsourced "3-5x faster" and "7% to 95% batchable" figures for DeferStateAndOwner with the actual mechanism: records carrying statecode, statuscode or ownerid are not batchable, so the gain is proportional to how much of the block carried them, and is nothing when none do. - Mentioned the new Builder checkbox and the batch fault-reporting fix.
The match-based display string fell back to a plain ToString() on the attribute value. Records deserialized from a data file carry no FormattedValues, so every OptionSetValue, Money or lookup rendered as 'Microsoft.Xrm.Sdk.OptionSetValue' - which made the per-row Created, Updated and Failed lines useless for identifying a record. Use the existing AttributeAsBaseType helper for the fallback instead.
Definitions commonly put state changes in their own Save=UpdateOnly block that exports nothing but statecode and statuscode. Deferring there strips every attribute off the record and leaves nothing to save. IsStateOwnerOnlyBlock turns the option off for such a block and says so in the log; HasAttributesBesidesStateOwner guards the same case per record so a mixed block still defers the records that have other data.
A record that matched more than one record in the target was reported over
two lines with no row number:
Import object matches 2 records in target database!
2019, 10
Every other result line in the block is prefixed "{0:000}", so the rejected
row was the only one whose number never appeared. Finding it in a 168-row
block meant diffing the printed sequence against the expected one.
It is now one line in the same shape as the other failures:
010 Match Failed: 2019, 10 matches 2 records in target database
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The README described StopOnError only at block level. Batching changes what has already happened when a run stops: with ContinueOnError=false the platform abandons the rest of the flight, so the run needs to say how many records that was. Verified on CRM 9.1 on-prem both ways. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A bulk request is transactional, so a fault rolls the whole batch back and nothing is written. The four bulk catch blocks nevertheless rethrew before reaching the per-record fallback, which produced two problems on any org that supports CreateMultiple / UpdateMultiple / UpsertMultiple: - No failing row was ever reported. Verified on an online org: an import of 48 records with BatchSize=20 and one over-length attribute logged zero "Create Failed" lines and zero "Created" lines. The only record named was the last one enqueued into the batch, which was innocent -- the outer catch labels the exception with whatever record the loop variable holds, and a batch-boundary flush holds the final one. At the default BatchSize=100 the operator is told "somewhere in these 100 rows", and told the wrong row number. The same definition and data on an on-premises org, which has no bulk messages and therefore goes through ExecuteMultiple, correctly reported "012 Create Failed: ..." and the abort count. - StopOnError's footprint became estate-dependent. Individual creates commit the rows ahead of the fault; so does ExecuteMultiple. A rolled-back bulk batch commits none of them, so the same import left 11 records behind on-premises and 0 online. Fall through to the per-record path in all four cases instead. Nothing was written, so re-running the rows is safe, and it is the only way to attribute the fault to a record. Each fallback already logs the failing row and then honours StopOnError itself, so the abort still happens -- one record later, with the record named, and with the rows ahead of it committed as before. This also removes a log line that claimed an action it did not take: the create and update paths logged "falling back to individual creates/updates" immediately before rethrowing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
GetEntityDisplayString builds the per-record label from the block's Match
attributes and reads each one with cdEntity.Contains(). The primary key is
carried in Entity.Id and is never present in Entity.Attributes, so a block
matching on the primary key hit the "<null>" initialiser for every record:
001 Updated: <null>
012 Update Failed: <null> cint_mms_mua_charge_period A validation error...
*** Error record: <null> ***
The Count == 0 fallback to cdEntity.Id.ToString() did not help, because one
(null) entry had already been added to the list.
EntityAttributesEqual already special-cases PrimaryIdAttribute the same way
when comparing records; this gives the display path the matching case, so
id-matched blocks name the record they are working on.
Pre-existing behaviour, not introduced by the batching work - but batching
made it far more visible, since the failing row in a batch is now reported
by name.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The four ExecuteMultiple loops log
StopOnError: aborting, N record(s) in this batch were not executed
before rethrowing, so the operator can see how much of the batch never ran.
The per-record loops had no equivalent: FlushCreatesIndividually,
FlushUpdatesIndividually, FlushUpsertsAsCreateUpdate, ApplyStatesIndividually
and FlushDeferredOwnerChanges reported the failing row and rethrew, leaving
the rest of the batch silently unaccounted for.
The gap predates this work, but the fallbacks used to be unreachable under
StopOnError, so it was never visible. Now that a faulted bulk request falls
through to the per-record path, an aborting import can end inside one of
these loops - and it did, on the first UpdateMultiple fault test: eleven rows
committed, row twelve named, and no word about the eight that never ran.
Each loop becomes an indexed for so it can report batch.Count - i - 1, the
same arithmetic the ExecuteMultiple loops use.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Neither Upsert nor UpsertMultiple reports as supported for the custom tables tested during this work, on an on-premises 9.1 org or an online one, so both branches fall straight through to Create/Update today. Record why they stay: support is per table, several out-of-the-box tables already carry Upsert, and detection is cached per entity, so an org that gains support picks it up with no change here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DeferStateAndOwner strips statecode, statuscode and ownerid off every record
up front, before the main pass knows whether that record will be written at
all. Records that go nowhere - no match under UpdateOnly, an ambiguous match,
or nothing created - keep an empty ActualId and were then reported as
failures by the deferred pass, each one on a line reading
Failed (deferred): <name> - ActualId not set
Those are normal outcomes the main pass has already reported, so drop them
before the deferred pass runs and say how many were dropped.
Measured on two estates against ShuffleMMSMUAPluginSteps with the attribute
switched on: online 68 such failures, all 68 accounted for by "Not creating";
on-prem 4, being 2 "Not creating" plus 2 ambiguous matches. The same data
imported without deferral reported Failed: 0.
The deferred pass also fed the block's own updated and failed counters, which
counted every deferred record a second time - a 67-record block summed
Updated + Skipped + Failed to 134. It now keeps its own counters and reports
them on their own line.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Batches are flushed from inside the record loop, as soon as the pending list
reaches BatchSize. When a flush throws, the exception surfaces in the catch of
whichever record happened to fill the batch - so under StopOnError the
*** Error record: <identifier> ***
line named the last record enqueued rather than the one that failed. A run
where record 012 faulted reported record 020.
Record the failing item's position and identifier as the batch is unwound and
let the per-record catch prefer that label. Each site keeps its own throw, so
the stack is unchanged, and the label is only set when StopOnError is on -
ContinueOnError already reports each failure where it happens.
Not covered: a whole-batch ExecuteMultiple Execute failure, where no single
record is at fault, and the delete path, which has no per-item identifier.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Batching is new in this branch - master has no BatchSize attribute at all - so shipping a default of 100 would silently move every existing definition onto CreateMultiple/UpdateMultiple. Those are a single transaction: one bad row rolls back the whole batch, where the previous per-record import would have failed only that row. Default BatchSize to 1 so batching is opt-in. The clamp in ImportDataBlock and the Count == 1 shortcuts in the four flush dispatchers already make 1 mean "no batching", so no new code is needed - only the three defaults that have to agree (the field initialiser, the DefaultValue attribute that controls serialisation, and the Builder Tag that controls omission) plus the XSD. Also drop DefaultBatchSize, which nothing ever read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two of these were wrong independently of the default change. The bullet claiming the default batch size was "reduced from 200 to 100" describes something that never happened - master has no BatchSize attribute and no prior default - and "no configuration changes required" is only true of capability detection, not of batching, which now has to be asked for. Also say plainly in the performance tip that CreateMultiple and UpdateMultiple are transactional, since that is the cost of opting in and the reason the default is 1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The "Prereq:" line was written before the block that resolves the
comparer and the version, so every run logged 0.0 and the next line
went on to evaluate the real version. Observed against both a
satisfied and an unsatisfied prerequisite:
Prereq: CinterosUtils ge 0.0
Prerequisite CinterosUtils ge 16.0.0.0 is satisfied
The comparer is rewritten there too (eqthis becomes eq, gethis
becomes ge), so both values are now logged after resolution.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There were no automated tests at all, and the batching work in this PR adds
six flush paths, a three-level fallback chain and positional response pairing
to ShuffleDataImport - none of it reachable from a manual Shuffle Runner run
without a live estate.
The repo has no core assembly, only shared projects, so the test project
imports the same two .projitems as the XTB project:
shared/Xrm.Shuffle.Core/Xrm.Shuffle.Core.projitems
Xrm.Utils.Core/Xrm.Utils.Core.Common/Xrm.Utils.Core.Common.projitems
Neither of those references System.Windows.Forms, System.Drawing,
DirectoryServices or System.Workflow, so the core compiles into the test
assembly with no WinForms and no XrmToolBox dependency. The framework
Reference list here deliberately omits those.
Shuffler is partial, which is what lets later fixtures reach its private
members through their own partial class file - no reflection and no
InternalsVisibleTo.
NUnit 3.14.0 with NUnit3TestAdapter 4.5.0, on PackageReference to match the
XTB project (the repo has no packages.config anywhere). The adapter's props
file is auto-imported through obj\*.csproj.nuget.g.props, so the adapter DLLs
land beside the test DLL and vstest.console.exe discovers them with no
/TestAdapterPath and no separate console runner.
Six smoke tests to start with. Two of them pin the new opt-in BatchSize
default from the first commit in this PR - one on the ctor, one through a
definition that never mentions the attribute. A third feeds an undeclared
attribute and expects XmlSchemaValidationException, which proves the embedded
schemas really loaded: ValidateDefinitionXml silently skips validation when
fewer than two schemas resolve, so without that test a broken resource name
would look like a passing suite.
Build the solution, not the csproj - the project platform is AnyCPU and the
sln performs the "Any CPU" mapping.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three steps between "Build solution" and "NuGet pack": find the VSTest console, run the test assembly, upload the trx. vswhere is called with a bare -find and no -requires or -latest. -find already lists only the instances that actually contain the file, so Build Tools counts as a hit, and there is no way for the newest instance to win the -latest race and then turn out to have no test platform. Verified locally against three side-by-side instances. PowerShell does not fail a step on a native tool's exit code, so the step throws on $LASTEXITCODE itself - without that, a red test would upload its trx and let the build go green. The upload runs under if: always() so the trx survives the throw. No extra restore step: the solution restore above already covers the test project. No /Platform either - the assembly is AnyCPU. CLAUDE.md no longer claims there are no automated tests. It now names the project, gives the local command, notes that the solution rather than the csproj is what you build, and says what is still only validated by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The core talks to the platform through IExecutionContainer and IOrganizationService, and neither has a test-friendly implementation: the only concrete container writes log files to C:\Temp. These are hand-rolled stubs rather than mocks, because IExecutionContainer is three properties and ILoggable is seven void methods. ScriptedOrganizationService can express what the interesting batch cases need and what a faked context cannot: a response collection shorter than the request list, responses arriving out of request order, and a capability probe that answers only for the messages a fixture declares. An unscripted message throws and names every request seen so far, so a fixture that routes down an unexpected rung says so. ShufflerTestShim is a partial of Shuffler, so the tests reach the private flush methods and the private pending-batch structs with no reflection and no InternalsVisibleTo. The flush calls sit on the nested batch classes rather than on the outer partial, because a containing type cannot reach a nested type's private members and the item lists have to stay private - their element types are private. CreateForTest also initialises guidmap and stoponerror, which the product only does inside ImportToCRM. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The counters an import reports come out of these loops, so a pairing bug does not look like a bug: it looks like a run that says it created more records than it did. The case that motivated this is the one 4f9233d fixed - a fault partway through a batch used to stop the accounting, leaving the records after it counted as neither created nor failed. One fault in twenty is now pinned as nineteen successes and one failure, with the totals adding up to the batch size. The rest cover the shapes the platform is allowed to return but a faked context cannot produce: response items out of request order, which is why the loops look them up by RequestIndex rather than by position, and a response collection shorter than the request list, which is what ContinueOnError=false leaves behind at the first fault. Those unanswered requests were never executed, so they must count as failures. Delete gets its own pairing tests because its loop is worded differently and treats a "does not exist" fault as success - the record is absent, which is what was asked for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Which rung of the fallback chain a batch actually took is invisible from the outside: CreateMultiple, ExecuteMultiple and a per-record loop all report the same Created count for the same data. So a batch that quietly fell through to the slow path looks exactly like one that did not, and these fixtures assert the requests that left the service rather than only the counters that came back. The two failure cases the product must keep apart are pinned separately. "This message does not exist on this org" is permanent, so it is remembered and the next batch of the same entity goes straight to ExecuteMultiple without asking again. "This batch faulted" says nothing about the next one and must not be cached. The capability probe is asserted to run once per entity and message, and a probe that cannot be answered at all is asserted to be read as a no rather than to fail the import. CreateMultiple and UpdateMultiple are one transaction, so a fault rolled every row back and nothing was written. The rows are therefore re-run one at a time even when StopOnError is set - that is the only way to name the row that faulted, and the per-record loop honours StopOnError itself afterwards. Upsert carries one rung more than the others, because ExecuteMultiple with UpsertRequest sits between UpsertMultiple and the Create/Update split. Two tests are deliberately written to the behaviour as it is rather than as it should be. A late "Upsert not implemented" fault discovered partway down an ExecuteMultiple batch counts the rows ahead of it twice, because the rung returns false after counting them and the caller re-runs the whole batch; and the capability query passes a logical name to primaryobjecttypecode, which holds a numeric entity type code. Both carry a comment saying a fix should make the test fail and be rewritten, rather than let it keep passing over changed behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five fixtures over a fake org, covering the decisions ImportDataBlock makes before any batch is flushed: which bulk message the capability probe picks, what a match attribute resolves to and what happens to each answer, when the upsert gate opens, which records are batchable, and when an identical record is skipped. Two things the harness had to learn. Seeded rows now carry their primary id attribute, because match queries always ask for it and a real retrieve answers with it populated. And an entity that has metadata must declare its attribute list: FakeXrmEasy answers a query naming an undeclared attribute with "The attribute X does not exist on this entity" even when every row carries it, so ShuffleTestContext assembles the list from the seeded rows and WithAttributes covers anything only the source records hold. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
DeferStateAndOwner strips statecode/statuscode/ownerid off each record so the
record itself becomes batchable, then applies them in a second pass once the
real ids are known. Eighteen fixtures cover the strip pass, the actual-id fill,
the state pass (one UpdateMultiple per entity, the SetState fallback, the
savedquery/duplicaterule exceptions), the owner pass, and the block-level
wiring that turns the option off for a block carrying nothing else.
Three of them are landmine markers - they assert what the code does today, and
a future fix should make them fail:
- OperationsSet2.Assign() swallows every exception and returns bool, so the
whole failure branch of FlushDeferredOwnerChanges is unreachable through the
fluent helper: a failed assign is counted as applied, logged as "Assigned
(deferred)", and StopOnError never fires.
- container.Principal(entity).On(change.Owner) sends
AssignRequest { Assignee = <the record>, Target = <the owner> } - the two
references are inverted. The swallowed exception above is what keeps this
invisible in production.
- A savedquery deferred at state 1 / status 1 is sent as SetState(1, 2) but
logged as 1/1.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Batching breaks the assumption the import used to rest on: that a record has its real id by the time the next record is prepared. Two mechanisms put that back, and neither had a test. The guid map, covered in GuidRemappingTests: MapGuid declines an empty, an unchanged and an already mapped id, which is exactly why RecordCreatedId exists alongside it - a deferred state or owner change still needs the id in the two cases the map skips. ReplaceGuids rewrites EntityReference lookups in place, notes a raw guid attribute it cannot rewrite, and refuses outright when ids are carried over. The two block-level cases are the point of all of it: a later block points at what the earlier block actually wrote, and a block of creates maps every id it assigned. The pending-create guard, covered in PendingCreateReferenceTests: it matches on the id of the source system rather than on what the queued entity holds, since the create branch blanks the id before queueing; it treats a raw guid the same as a lookup, because it cannot tell them apart and guessing wrong loses data; and a hit flushes the batch early so the ids exist before the lookup is written. One test records the cost honestly - an early flush of a single queued record goes out as a plain Create, because the dispatcher shortcuts a batch of one. Three new shim members reach the private methods: TestReplaceGuids, TestMapGuid and TestRecordCreatedId. 131 tests, all green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A batch moves the failure away from the record that caused it. By the time the platform answers, the import loop is several records further on, so its catch would name whichever record happened to fill the batch. StopOnBatchError exists to carry the right label out of the flush; these fixtures pin that mechanism and the two routes that get around it. BatchErrorReportingTests covers the label itself and the lowest upsert rung, FlushUpsertsAsCreateUpdate, which has to infer create-versus-update from the fault a create came back with. One case is written as a known defect: the duplicate check is case sensitive, so a fault saying "Duplicate record found" is reported as a create failure rather than becoming an update. LogMessageTests covers the throws that leave ImportDataBlock by a route the per-record catch cannot see - the delete-all pass above the record loop, and the three flushes that empty the pending batches after it closes. It also pins the mid-loop case where no label is set and the wrong record is blamed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two log lines in ShuffleDataImport sit inside #if DEBUG and print the match query as FetchXml, which ConvertToFetchXml gets by sending a QueryExpressionToFetchXmlRequest. FakeXrmEasy 1.x has no executor for that message and throws, and the scripted service throws for anything a fixture has not scripted, so under Debug every fixture reaching a matched block died before the match query ran: 135 of 149. Release compiles those lines out, so the suite was green there and red in Visual Studio, which defaults to Debug. CI builds and tests Release only, so it never saw this. Both services now answer the message before recording it and return a token FetchXml string - the product uses the value for nothing but the log line. Not recording it is what makes a fixture observe the same request sequence in both configurations. Debug and Release are now both 149/149. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The workflow built and tested Release only, so CI could stay green while Test Explorer was red. That is not hypothetical: ShuffleDataImport logs the match query as FetchXml inside two #if DEBUG blocks, which sends a QueryExpressionToFetchXmlRequest the test doubles have to answer. Release compiles those lines out, Debug does not, and Visual Studio defaults to Debug - which is how fourteen failures reached a developer without CI noticing. Add a Debug build and test leg after the Release one, writing its trx into the same results directory (the upload step already globs *.trx). The Release trx gains a matching name so the two artefacts are told apart. Both legs build the solution rather than the test csproj, because the project platform is AnyCPU and only the sln maps Any CPU onto it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The test csproj started as a copy of the XTB project reference list, which carries assemblies the test assembly has no reason to load: System.Activities, System.IdentityModel, System.ServiceModel.Web, System.Web and friends. Thirteen of them are unused - the solution builds clean and all 149 tests pass in both configurations without them. System.IO.Compression.FileSystem stays: ShuffleSolutionImport reaches for ZipFile, and removing it is the one that breaks the build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both changes in this PR that sit outside import batching had no test at all. SelectAttributes was removing keys from the collection it was walking, which shifted the remaining entries down and skipped the one after every removal, so a run of adjacent unwanted attributes came out half-filtered into the exported file. It is now a two-pass collect-then-remove. The fixture pins that, the wildcard forms a definition can write, and the ordering that makes a filtered column reappear as null. SendText was handing its arguments to the logger along with a string it had already formatted, so the logger formatted it again. A record identifier holding a placeholder was rewritten with a value from the same line, and one holding a lone brace threw FormatException out of the log call. One test marks the part that is still exposed: the first format pass runs even with no arguments, so an already-interpolated message carrying a brace still throws. 169 tests, green in Debug and Release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
What this does
Import gains bulk-message batching. Where a block's records can be sent together, Shuffle now uses
CreateMultiple/UpdateMultiple/UpsertMultiplewhere the entity supports them, and falls back toExecuteMultipleRequestwhere it does not — so the same definitions work unchanged against both online Dataverse and on-prem 9.1.BatchSizeis exposed in the schema and the Builder, and now defaults to 100 rather than effectively 1.Alongside that: Multi-Select OptionSet handling, deterministic export ordering, clearer import logging (real values instead of SDK type names), a
DeferStateAndOwneroption for high-throughput imports, and GitHub Actions for build and release.Batching engages only for
Match PreRetrieveAll="true"Worth knowing before raising
BatchSizeand expecting a speed-up. A block withoutPreRetrieveAlldoes one match query per record, which forces the whole block down the individual path no matter whatBatchSizesays. Measured across a real definition set: the blocks that carryPreRetrieveAllbatched, the ones that did not stayed individual. Nothing is broken by this — it is how matching has always worked — but the attribute is the switch that makesBatchSizemean anything.Fixes in this PR
Deferred state and owner changes on records that were never written.
DeferStateAndOwnerstripsstatecode,statuscodeandowneridoff every record before the main pass knows whether that record will be written at all. Records that go nowhere — no match underUpdateOnly, an ambiguous match, or nothing created — kept an emptyActualIdand were then reported by the deferred pass asFailed (deferred): <name> - ActualId not set. Measured on two estates: 68 such failures online, all 68 accounted for byNot creating, and 4 on-prem, being 2Not creatingplus 2 ambiguous matches; the same data imported without deferral reportedFailed: 0. Those entries are now dropped before the deferred pass, with a count. The deferred pass also fed the block's own counters, double-counting every deferred record — a 67-record block summedUpdated + Skipped + Failedto 134 — and now keeps and reports its own.*** Error record ***naming the wrong record. Batches flush from inside the record loop, as soon as the pending list reachesBatchSize, so a fault surfaced in the catch of whichever record filled the batch rather than the one that failed. A run where record 012 faulted reported record 020. The failing item's position and identifier are now recorded as the batch unwinds and preferred by that catch. Whole-batchExecuteMultiplefailures and the delete path are not covered — neither has a single record at fault.Testing
Exercised against two estates — an on-prem 9.1 IFD org (
Multiple support: False, falls back toExecuteMultiple) and an online Dataverse org (True, uses the bulk messages) — with a real ~37-block definition set plus purpose-built fault definitions coveringStopOnError,ContinueOnError, bulk-to-individual fallback, ambiguous matches and eachSavemode. Both estates produced equivalent block counts, and a second run of the same data was idempotent (all skips reported(Identical)).🤖 Generated with Claude Code