fix: stop corrupting violation URLs with position info#226
Merged
Conversation
analyze-markuplint and analyze-textlint validators report a position alongside each violation, but Violation had no field for it, so both plugins concatenated it onto url instead (issue #225).
replaceAnalysisViolations resolves each violation's url against content_items/url_refs and throws if any url can't be resolved. The position suffix that analyze-markuplint/analyze-textlint used to append to url never matches a real page url, so any archive analyzed with those validators enabled could carry violations that can never resolve (issue #225). replaceAnalysisViolations now: - accepts explicit line/col fields and persists them - when line/col are absent, parses the legacy `<url> (<line>:<col>)` suffix back into a clean url plus line/col before resolving, repairing both freshly-analyzed and legacy-JSON-backfilled data - self-heals analysis_violations tables provisioned before line/col existed, adding the nullable columns via hasColumn-guarded ALTER TABLE before rewriting rows, so no operator intervention is needed create-adjunct-tables.ts gains the line/col columns for newly-created tables. retarget-legacy-fk-tables.ts's copy-back (used by the pre-0.13 migration script) now null-fills columns on an explicit allowlist when the staged table predates them, instead of aborting — safe only because those columns cannot have held a value before they existed; any other missing column still aborts loudly.
backfillAnalysisViolationsFromJson passes legacy analysis/violations.json data straight through to replaceAnalysisViolations, whose accepted shape now includes optional line/col (issue #225). No behavior change here — the repair logic lives entirely in replaceAnalysisViolations.
eachPage built url as `${url.href} (${line}:${col})`, corrupting the page
URL that replaceAnalysisViolations resolves violations against, so any
markuplint violation could make analyze throw (issue #225).
url is now the plain page URL, matching analyze-axe; line/col are
reported through Violation's new dedicated fields instead.
eachPage built url as `${report.url} (${line}:${column})`, corrupting the
page URL that replaceAnalysisViolations resolves violations against, so
any textlint violation could make analyze throw (issue #225).
url is now the plain page URL, matching analyze-axe; line/col are
reported through Violation's new dedicated fields instead.
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
analyze-markuplintandanalyze-textlintbuiltViolation.urlas`${url.href} (${line}:${col})`, corrupting the page URL thatreplaceAnalysisViolationsresolves againstcontent_items/url_refs.Any archive analyzed with either validator enabled (and at least one
violation) could make
analyzethrow, since the corrupted URL can neverresolve.
url(matchinganalyze-axe) and reportthe position through
Violation's new optionalline/colfields.analysis_violationsgains nullableline/colcolumns. Existingarchives — including ones whose
analysis_violationstable predatesthese columns, and legacy
analysis/violations.jsonpayloads stillcarrying the corrupted
<url> (<line>:<col>)form — are repairedautomatically the next time
replaceAnalysisViolationsruns (liveanalyze, or the backfill path), with no operator intervention.Design note
Column additions to an existing table normally go through a version-gated
migration script.
analysis_violations.line/colself-heals instead(
hasColumn+ALTER TABLE ADD COLUMNinsidereplaceAnalysisViolationsitself), because the columns are nullable, additive, and this function is
the table's only write path. ARCHITECTURE.md's invariants list now states
this as a general, narrow exception (not a one-off hack) so it isn't
mistaken for drift in a future review.
Test plan
yarn buildyarn lintyarn testlegacy corrupted-URL repair, explicit line/col taking priority over URL
parsing, axe-style violations persisting with
line/colasnull,self-healing an
analysis_violationstable that predates the columns,and
retargetLegacyFkTables(used byscripts/migrate-to-0.13.mjs)null-filling the same columns for pre-0.13 archives while still
aborting loudly on any other unexpected missing column.
Closes #225