[import] Report the syntax errors truncating a textual import - #2511
[import] Report the syntax errors truncating a textual import#25111wgrumph wants to merge 1 commit into
Conversation
|
Pushed a follow-up commit repairing three defects found while re-reading the first one, plus the wording of the public texts. The validation pass could deadlock, and its timeout was unreachable. The CLI serializes an AST on its standard output during that pass for every document it finds valid, that pipe was never drained, and the standard error was joined before the timed wait. A document large enough to fill the pipe blocked the upload forever instead of hitting the bound. The output of the pass is now redirected to Substring matching could reject a valid document. The CLI renders a diagnostic as An inconclusive pass silently restored the old behaviour. A timeout, a launch failure or a failed read all returned an empty list, which the caller read as "the document is complete", so the truncated AST was accepted. The pass now has three outcomes: complete, truncated, and unavailable. The AST is withheld and an operational error is reported for the last one, and the unrelated diagnostics are only ignored after a pass that actually completed. Tests. Every case importing a document now asserts that the Texts. The claim that the validation pass "does not serialize the AST" was wrong and has been removed from the issue, this description and the release note: the pass does serialize one for a validation-clean document, and therefore does pay the
|
|
Startup regression from my previous commit, fixed in The commit repairing the validation pass added a second, package-private constructor so the tests could shorten the timeout. Spring only selects a constructor implicitly when a class declares exactly one, so The constructor taking the configured CLI path is now annotated with Added
|
|
Second review round, fixed in Abnormal termination was accepted as "document complete". A Coloring could hide the sentinel. With Diagnostics are now split statefully. The CLI copies the source range verbatim, newlines included, so a continuation line can look exactly like a diagnostic header. A line only starts a new diagnostic when the brackets of the previous one are closed. Where that cannot be decided — the words appear at a line start, no diagnostic header carries them, and the brackets of the report do not balance — the outcome is unavailable rather than clean, so the ambiguity fails closed. The report is bounded, and cleanup is tree-wide. The stream is still drained to its end so the CLI can terminate, but at most 64 diagnostics are retained, each abbreviated to 1 KiB; the parsing pass keeps at most 64 KiB of its own stderr. A pass that has to be destroyed is now destroyed together with Tests. Seven added, all in Texts. The issue and this description now say the extra parse is one per
One limitation worth stating: the CLI's report is a human rendering with unescaped source text, so the bracket balance is a heuristic, not a grammar. It is deliberately biased towards withholding the AST when it cannot decide. A machine-readable diagnostic output from the CLI would remove the guesswork entirely, if that is something the SysIDE side would consider. |
|
One more, in The helper reading the returned AST wrapped the stream failure in an This was in the branch since the first repair commit. My earlier runs built the module with |
|
@1wgrumph thank you for providing this PR. Please take a look at previous commits, and at https://doc.mbse-syson.org/syson/v2026.7.0/developer-guide/index.html#_contribute_a_change_in_the_codebase. We are also close to the 2026.9.0 release (this Wednesday), so this PR will be part of 2026.11.0 release. Thank you for your understanding. Regards, |
| return available; | ||
| } | ||
|
|
||
| @DisplayName("Given a valid document, when it is converted, then its whole AST is returned without any error") |
There was a problem hiding this comment.
- Given => GIVEN
- when => WHEN
- then => THEN
as for every other test in the codebase please
| /** | ||
| * Writes the given script as the CLI the conversion under test calls. | ||
| * | ||
| * @param directory the directory the script is written into |
There was a problem hiding this comment.
formatting is wrong, please use our checkstyle convention from https://github.com/eclipse-syson/syson/blob/main/backend/releng/syson-resources/checkstyle/CheckstyleConfiguration.xml please
The parser used to produce the AST of a textual import reports neither a non-zero exit code nor anything on its standard error when a SysML file contains a syntax error that leaves an unparsed tail; it prints an AST covering only the part of the file located before the error. The import succeeded with an empty report while everything after the error was silently missing from the resulting model. A second, validation-only pass of the CLI now runs after the parsing pass. Its diagnostics are read as a parsing problem in their own right: the exit code decides first, coloring is forced off, a diagnostic spans lines only while its quoted source range is open, an unbalanced report is unavailable rather than clean, and no over-cap physical line is ever held whole. Both passes share one bound; the process tree recorded while a pass ran is destroyed with it, and the readers of its output are owned until the conversion ends, including when the calling thread is interrupted. Such a file is now reported as invalid, with the parser diagnostics, and is no longer imported. Bug: eclipse-syson#2510 Signed-off-by: William Rumph IV <1wgrumph@gmail.com>
a34221b to
8d95eef
Compare
|
Reshaped into one commit in the format from the developer guide ( |
|
Some new remarks:
So “a second project” means the patch goes beyond “run validation and detect truncation” into designing a bounded, multiline diagnostic protocol parser plus adversarial process cleanup.
|
2 participants
Title: [import] Report the syntax errors truncating a textual import
Body:
Fixes #2510
A SysML file containing a syntax error is currently imported silently and truncated:
SysmlToAstruns the parser asdump <file>, which exits with 0 and prints nothing on its standard error in that case, while printing an AST that only covers the part of the document located before the error. The upload succeeds with an empty report and every element declared after the error is missing from the model.--validatecannot be added to the call producing the AST: the CLI then prints no AST at all as soon as it finds any validation error, and exit code 1 does not mean the document is invalid (11 of the 18 import fixtures in this repository exit with 1, because the standard library is not resolved when the CLI runs standalone and because semantic checks are reported for many valid documents). Reacting to the syntax diagnostics alone is not correct either:convertBooleanTest/boolean.sysmlreports one onconstant attribute ro;and is nevertheless parsed completely.This change keeps the
dumpcall unchanged and runs a separatedump --validate --stdlib nonepass once the AST has been produced. Only theExpecting end of filediagnostic is used, since the parser emits it when it stopped before the end of the document and left an unparsed tail out of the printed AST. It is matched as^line \d+: Expecting end of file\bon the message rather than searched in the whole reported line: the CLI renders a diagnostic asline <number>: <message> [<source text>], so an unrelated diagnostic quoting a source range that contains those words would otherwise reject a document that was parsed entirely. The import then fails and reports the associated parser diagnostics. The other diagnostics are ignored on purpose, and the reason is documented in the code.The validation pass has three outcomes rather than two. When it cannot be completed, because it timed out or because the CLI could not be run, the AST is withheld and the failure is reported: nothing then shows whether the document was parsed entirely, so importing the AST would restore the silent truncation this change is about. The two CLI invocations of a conversion share a single 60 second bound, and both kinds of wait a conversion makes are taken against it: waiting for a process to exit, and waiting for the readers of its output. The second one matters as much as the first, because the exit of the CLI does not end them when a child it spawned kept the pipes it inherited. Giving up on a pass is bounded separately, since neither of its two stages can be charged to a budget that is already spent when it starts: destroying what is left of the tree has five seconds, and waiting for the readers that destruction releases has five more, so a conversion can return up to ten seconds after its own deadline. An interruption of the calling thread takes that same way out rather than propagating: the interruption is held while the process tree is destroyed and its readers are ended or cancelled, and handed back to the caller afterwards, since a caller that stops waiting is not a reason to leave a parser behind.
The extra pass costs one parse per
SysmlToAst.convertcall, which covers the document upload, New object from text and the expression editor. The CLI also serializes an AST on its standard output during that pass whenever it finds no validation error, so it does pay theJSON.stringifycost addressed by #2042 for such a document. That output is of no use here, so it is redirected toDISCARD, which also keeps the CLI from blocking on a pipe nobody reads. On a 117 KB, 6000 line model thedumpcall takes about 1.1 s and the validation pass about 1.3 s.Reading that report is treated as a parsing problem in its own right, because a misread report either rejects a valid document or, worse, lets a truncated one through. The exit code decides first:
0means nothing was reported,1is only accepted together with theThere are validation errors:line the CLI prints before its diagnostics, and any other way of ending leaves the outcome unknown. Coloring is forced off for both invocations, since the CLI ignoresNO_COLORas soon asFORCE_COLORis set and an escape sequence in front of a diagnostic would hide it. The source text the CLI appends to a diagnostic is copied verbatim and may span several lines, so a line only starts a new diagnostic when the brackets of the previous one are closed; a report whose brackets do not balance cannot be split reliably and is treated as unavailable rather than as clean. The report is drained to its end so the CLI can terminate, but only a bounded number of abbreviated diagnostics is kept, and no over-cap physical line is held whole: a quoted source range containing no line separator is as long as the document it comes from, so such a line is drained while only its beginning is retained. A pass that has to be destroyed is destroyed together with the descendants observed while it was running, under a bounded wait: a descendant is only reachable through a parent that is still alive, so the tree is sampled from one millisecond, backing off to every 100 ms, rather than read at cleanup time, when the CLI may already be gone. A child spawned and orphaned between two samples is not reachable at all; what protects the conversion from it is that the reader waits are bounded, not that the child can be destroyed.SysmlToAstTestcovers the truncated document, a valid document, a document the parser recovers from despite a syntax diagnostic, a valid document whose source text is quoted back by a diagnostic containingExpecting end of file, a validation pass printing more than a pipe buffer, one that never terminates, one that crashes, one that exits with an error but no diagnostic envelope, one that colors its report, a diagnostic whose quoted source range spans several lines, a report whose brackets do not balance, a report of megabytes of diagnostics, a diagnostic quoting megabytes on a single physical line, the same one with its quoted range left open, a pass that leaves a child behind, a pass whose child outlives it while holding the pipes it inherited, and a conversion interrupted while the CLI is still running, which asserts that the CLI is gone and that the interruption reached the caller. The three cases that import a real document through the real CLI assert that theBdefinition, which is declared after the syntax error, is present in the returned AST, and not only that an AST was returned; the cases importing through a stub assert the AST that stub prints, since what they cover is how the report of the validation pass is read rather than what the parser produces. The tests of a CLI that does not terminate on its own carry an outer timeout of their own, so that a conversion that stopped being bounded fails them instead of stalling the test worker. The class is skipped when no Node runtime is available, since the embedded CLI is a Node script; the continuous integration provides one.SysmlToAstBeanTestcreates the component in a Spring context: the class now declares a second, package-private constructor so the tests can shorten that bound, so the constructor to inject is pointed at with@Autowiredrather than left implicit.Reproduced on
bb52f47with the embedded SysIDE CLI 0.9.0 and Node.js v22.23.2.PLEASE READ ALL ITEMS AND CHECK ONLY RELEVANT CHECKBOXES BELOW
Auto review
Project management
priority:andpr:labels been added to the pull request? (In case of doubt, start with the labelspriority: lowandpr: to review later)area:,type:)Changelog and release notes
CHANGELOG.adoc+doc/content/modules/user-manual/pages/release-notes/YYYY.MM.0.adocbeen updated to reference the relevant issues?CHANGELOG.adoc?CHANGELOG.adoc?doc/content/modules/user-manual/pages/release-notes/YYYY.MM.0.adoc?Key highlightssection indoc/content/modules/user-manual/pages/release-notes/YYYY.MM.0.adoc?Documentation
Tests