fix(compare): stop scripting a view, routine or trigger whose definition could not be read

[datlechin]

Stacked on #3063

Summary

Compare & Sync treated "could not read this definition" as an empty definition. A MySQL account without SHOW VIEW lists every view and is refused each SHOW CREATE VIEW. The comparison scored that empty text as Differs against the target's real view, suggested a replace, and the script it built was DROP VIEW with nothing after it. Two unreadable reads of one object scored Identical. A failed procedure/function list read as "no routines", so every routine on the target was offered as a DROP.

Now a view, procedure, function or trigger whose definition cannot be used lands in Could Not Compare with the reason. Nothing is scripted for it, and a list that cannot be read stops the comparison. Copy To reads the same way and shows the same reasons.

Root cause

• CompareMetadataService.viewDefinitions did try? fetchViewDefinition and then ?? "". routineReads did the same with fetchRoutineDDL, and with (try? fetchRoutines) ?? [] for the whole list. perTableTriggerReads skipped a table whose fetchTriggers threw, and a trigger with no definition fell back to statement, which is the action body.
• RoutineSourceRead.source: String had no way to say "unreadable". SourceObjectDiffEngine compared "" as text, and SourceObjectSyncBuilder turned .alter into DROP plus sendableStatements(""), which is []. The DROP ran alone.
• The listing definition was used before fetchRoutineDDL. DuckDB lists a macro's parsed expression ((a + b), SELECT 1 AS x) rather than a statement, so a replace ran DROP FUNCTION and then a bare SELECT.
• A source-only unreadable object was .create with an empty body, and the re-read before the script compared [""] with [""] and passed it.

What changed

• RoutineSourceRead.failure, like TableStructureRead.failure. The view, routine and trigger reads move into nonisolated statics in CompareMetadataService+SourceDefinitions.swift (readViewDefinitions, readRoutineDefinitions, readTriggerDefinitions), which are the test seam and are shared by Compare and Copy To.
  • A view always asks fetchViewDefinition, and a routine always asks fetchRoutineDDL. A trigger uses its listed definition when it has content, otherwise fetchTriggerDDL, and never statement.
  • A driver error becomes that object's failure, with the driver's message, and is logged. A CancellationError, or any error once the task is cancelled, propagates instead.
  • A failed fetchRoutines, or a failed per-table fetchTriggers (after the existing whole-schema fallback), throws CompareSyncError.readFailed naming the connection and the table.
• SourceDefinitionDefect is the one rule, used by the diff engine, the builder backstop and Copy To. The diff engine checks each side in its own engine's grammar, as the design said. The builder and Copy To check in the target's grammar, which is what they send. A structure script is only generated within one engine family (the same type, or MySQL and MariaDB, CompareSyncEngineFamily), and within a family the two grammars agree. It reports .unreadable(reason) for a failed read, .empty when the text has no sendable statement (blank or comment only), and .notACreateStatement when the first sendable statement's first code word, past comments, is not CREATE. SQLScriptText.leadingKeyword(of:) reads that word with the engine's own comment rules, so a SQL Server module with a header comment above its CREATE still counts as a statement.
• SourceObjectDiffEngine builds one result per object, matched or not. A defect on either side sets comparisonError, naming the side ("The source's definition could not be read: …"), and the text is never compared. CompareObjectResult then gives .skip / [.skip], and CompareReport already keeps such rows out of comparable, the counts, Select All and the script. The readable side's definition still shows in the detail pane. The name-only note is gone, along with its catalog key.
• SourceObjectSyncBuilder.build now throws. .create and .alter refuse a definition that fails the same rule. They log a fault and throw unsupportedOperation naming the object, so a miss upstream is visible rather than a silent []. .drop needs no definition.
• StructureChangeGuard.refusal(expected:actual:unreadable:): an object selected at compare time that is uncomparable on the re-read is refused with "could not be read again" and its reason, rather than "changed after it was compared".
• CompareRunner reads definitions only for the ticked view kinds, filters every source-defined result by the ticked kinds (an unticked kind's unreadable object would otherwise appear under Could Not Compare), and passes the re-read's uncomparable reasons to the guard.
• Copy To: ObjectCopyPlanner.sourceDefinitions goes through the same statics inside one withMetadataDriver call. definitionOutcome maps the shared rule to a skip. An unreadable read keeps the driver's reason, an empty one gets "The source reports no definition for it.", and a body gets the existing not-executable reason. A list that throws mid-plan does not fail the plan: only the selected procedures and functions, or the selected triggers, are skipped, each carrying the list's error, and the tables and views still copy. A cancelled list cancels the plan. ObjectCopyEligibility.isExecutableDefinition (a bare hasPrefix("CREATE")) is replaced by the shared rule.
• CHANGELOG (Fixed), docs/features/compare-sync.mdx, docs/features/copy-objects.mdx. The copy-objects page no longer lists Oracle among the drivers that answer a view with its SELECT: OraclePlugin.fetchViewDefinition has prepended CREATE OR REPLACE VIEW for a while.

Measured

• MySQL 8.4.11 (throwaway server in the lane scratchpad, lanes/compare_unreadable/measure-mysql84.txt):
  • cu_bare (SELECT only): information_schema.TABLES lists v as VIEW, and SHOW CREATE VIEW fails with ERROR 1142 (42000): SHOW VIEW command denied to user 'cu_bare'@'localhost' for table 'v'. ROUTINES and TRIGGERS return 0 rows with no error.
  • cu_select (SELECT, EXECUTE): the same 1142 on the view. Routines: 2 listed, but SHOW CREATE PROCEDURE returns Create Procedure: NULL, which MySQLPluginDriver.fetchRoutineDDL turns into insufficientPrivilege. Triggers: 0 listed.
  • cu_showview (SELECT, EXECUTE, SHOW VIEW, TRIGGER): the view definition is returned, 1 trigger is listed, and the procedure body is still NULL.
• DuckDB 1.5.4 (measure-duckdb.txt): duckdb_functions().macro_definition is (a + b) for a scalar macro and SELECT 1 AS x for a table macro, which is what DuckDBPluginDriver.fetchRoutines lists as definition.
• PostgreSQL was not re-measured; the design's measurement on 17.11 stands: pg_get_viewdef, pg_get_functiondef and pg_get_triggerdef return full text even without schema USAGE, so on PG only an error such as an object dropped mid-read reaches this path.
• Every schemaCompare driver that lists a routine definition (only DuckDB) or overrides fetchRoutineDDL was read. Each fetchRoutineDDL returns a listed definition that is already a statement, or fetches the DDL by name, so always asking it costs no extra round trip anywhere except DuckDB, which is in-process.

Tests

verify.sh test on 406f80c, 18 suites, 230 executed, 230 passed: SourceObjectDiffEngineTests 22, SourceObjectHazardTests 2, CompareSourceDefinitionReadTests 10 (new), SourceObjectSyncBuilderTests 17, StructureChangeGuardTests 23, CompareReportUnreadableTests 2, CompareMetadataReadPlanTests 13, CompareResultGroupingTests 11, CompareRunClaimTests 17, StructureDeclaredTypeCompareTests 11, ObjectCopyEligibilityTests 18, ObjectCopyPlannerOrderingTests 21, ObjectCopyPlannerSourceDefinitionTests 8 (new), ObjectCopySelectQueryTests 11, ObjectCopySequenceStatementTests 5, SQLScriptTextTests 23, SchemaSyncScriptBuilderTests 14, SQLExportScriptTextTests 2.

Red without the fix: I mutated each fix point back to the old behaviour, ran again and got 24 failures, every one of them a new or rewritten test:

• Views, routines and triggers read with failure: nil (the old swallow) turns red testAViewTheDriverRefusesCarriesTheDriversReason, testARoutineWhoseDDLCannotBeReadCarriesTheReason, testATriggerWithoutADefinition… and the three Copy To reason tests.
• (try? fetchRoutines) ?? [] and (try? fetchTriggers) ?? [] turn red testARoutineListingThatFailsStopsTheRead, testAPerTableTriggerListingThatFailsStopsTheRead, testAFailedRoutineListingSkipsTheRoutinesAndKeepsTheRest, testAFailedTriggerListingSkipsTheTriggers and testACancelledListingCancelsThePlan.
• The listing definition used before fetchRoutineDDL turns red testARoutineIsReadThroughItsDDLEvenWhenTheListingCarriesABody, and the fallback to trigger.statement turns red testATriggerWithoutADefinitionIsReadThroughItsDDLAndNeverItsStatement.
• A diff engine that never sets comparisonError turns red the seven unreadable cases in SourceObjectDiffEngineTests, including the one through CompareReport (0 differences, nothing comparable).
• A builder guard that never fires turns red both builder refusal tests.
• A guard that ignores unreadable turns red testAnObjectThatCouldNotBeReadAgainIsRefusedWithItsReason.
• A leadingKeyword that does not skip comments turns red both SQLScriptText tests and testACreateStatementIsRunnableBehindALeadingComment.

Follow-up c5e000c (review finding 1) adds testEachSideIsReadInItsOwnEnginesGrammar and testASQLServerModuleStoredAfterAlterOrCreateOrAlterIsReadable. The first is red with the source checked in the target's grammar (1 of 24 failed). Rerun on c5e000c: SourceObjectDiffEngineTests, SourceObjectHazardTests, SourceObjectSyncBuilderTests, StructureChangeGuardTests, CompareReportUnreadableTests and CompareSourceDefinitionReadTests, 78 executed, 78 passed.

testAnObjectWithNoDefinitionCarriesANote pinned the defect and is replaced by testAnUnreadableObjectOnlyOnTheSourceIsNeverCreated (comparisonError, [.skip]). Existing SourceObjectDiffEngineTests fixtures that used bare bodies (BEGIN END, SELECT 1) as definitions now use CREATE FUNCTION …, because a bare body is now correctly unreadable. Their assertions are unchanged.

Build: PASS. Lint: verify.sh lint on all 19 changed Swift files. Its one finding is single_test_class on SourceObjectDiffEngineTests.swift, which has held two test classes since before this branch. Docs: verify.sh docs PASS. No PluginKit or plugin change, so no ABI check or plugin build applies.

Before / After

Screenshots to be added. States to capture, from a MySQL source signed in as an account with SELECT only on a schema holding view v, compared against a target whose v differs:

1. Before: Definitions for v shows an empty source against the target's CREATE VIEW. The row reads Different and is included by Select All, and Script shows DROP VIEW with no CREATE after it.
2. After: v sits under Could Not Compare with "The source's definition could not be read: SHOW VIEW command denied…", its Include checkbox is dimmed, and Select All leaves it out.
3. After: with Procedures ticked and the source's routine list failing, the error banner reads "The procedures and functions in … could not be listed: …".
4. After: Copy To review step, with a selected view skipped with the server's reason.

Review findings

• Source definition checked in the target's grammar (fixed in c5e000c). Comparing across engines is allowed as a read (CompareSyncEngineFamily.swift:6-9), so a MySQL view whose definition opens with a # comment, compared against a PostgreSQL target, was misread as not a CREATE and filed under Could Not Compare. The diff engine now checks the source with sourceScriptText. The builder backstop and Copy To keep the target's grammar, because that is what they send, and scripts are only generated within one engine family.
• "A SQL Server module last changed with ALTER reads back starting with ALTER" (dismissed, no driver change). SQL Server stores the word ALTER as CREATE: after ALTER PROCEDURE Test1A AS SELECT 3;, OBJECT_DEFINITION returns /* comment */ CREATE PROCEDURE Test1A AS SELECT 3;. CREATE OR ALTER is stored with CREATE in its original case and OR ALTER removed (CrEaTe /*Y*/ PROCEDURE Test1B). Both come from Solomon Rutzky's tested write-up, which covers stored procedures, functions, views and triggers: Stored Procedure / Function / View / Trigger Definitions Can Be Wrong, Even If sp_rename Was Never Used. Microsoft's sys.sql_modules describes definition only as "SQL text that defines this module" (the same text OBJECT_DEFINITION returns), and says nothing on this point. MSSQLPluginDriver.fetchTriggerDefinition already assumes the stored text says CREATE TRIGGER. SQL Server could not be measured here. The leading comment in that stored text is why the rule skips comments, and both stored shapes are pinned by testASQLServerModuleStoredAfterAlterOrCreateOrAlterIsReadable.

Critique points not taken

• "File the MySQL listing defect as an issue before merge": the lead asked that no issues be opened, so it is listed below instead, and this PR does not claim that repro is fixed for routines and triggers.
• The design's SourceObjectSyncBuilder backstop "returns []": it throws instead (and logs a fault), per the critique.

Deliberately not fixed here

• MySQL privilege-filtered listings. information_schema.ROUTINES and TRIGGERS return 0 rows with no error for an account without EXECUTE or TRIGGER (measured above), so MySQLPluginDriver.fetchRoutines (Plugins/MySQLDriverPlugin/MySQLPluginDriver+Routines.swift:10) and fetchAllTriggers (:66) answer "none". The other side's routines and triggers still show as only on that side, which means DROP suggestions on the target. The docs now say so. It needs a driver-side privilege check.
• DuckDB table macro DDL. DuckDBPluginDriver.fetchRoutineDDL (Plugins/DuckDBDriverPlugin/DuckDBPluginDriver+Routines.swift:63) writes CREATE OR REPLACE MACRO main.t1() AS SELECT 1 AS x;, which DuckDB 1.5.4 refuses with Parser Error: syntax error at or near "SELECT". It needs AS TABLE for a table_macro. DuckDB has supportsTransactionalDDL == false, so a Compare replace of a differing table macro runs DROP FUNCTION and then that failing CREATE. Measured: 0 macros left afterwards. The run now reports the failure where it used to report success, but the macro is still gone. This is a registry plugin fix.
• Copy To's target listings swallow errors. ObjectCopyCatalog (TablePro/Core/ObjectCopy/ObjectCopyCatalog.swift:86, :96) uses (try? fetchRoutines) ?? [] and (try? fetchAllTriggers) ?? [], and :96 asks fetchAllTriggers even where providesBulkTriggerFetch is false (Oracle, Dameng), so those targets never report an existing trigger. Replace then attempts a CREATE over an existing object and fails. Nothing is lost, but the reason is wrong.
• Sequence toggle. Objects to Compare lists Sequence (CompareOptionsView.swift:67 iterates CompareObjectKind.allCases), but CompareRunner.sourceDefinedResults has no read path for it, so ticking it compares nothing.