Repository navigation
CP-314177: idl: check the two error lists agree - #7317
Open
GabrielBuica wants to merge 1 commit into
Open
GabrielBuica wants to merge 1 commit into
GabrielBuica wants to merge 1 commit into
Conversation
An API error is declared in two places: add_error in ocaml/xapi-consts/api_errors.ml makes it raisable, and the error function in ocaml/idl/datamodel_errors.ml declares its parameters and puts it in the generated SDK and the error reference. Nothing checked that both happened, so a code with only the first compiles, is raisable, and reaches a client as a string that appears in no reference. Add test_error_registration to the tests stanza under ocaml/idl. It folds the two lists against each other and names any code present in one and absent from the other. Both are read at run time rather than parsed out of the source, because api_errors.ml builds the AUTH_ENABLE_FAILED_* family by concatenation and those codes have no string literal for a textual lint to find. Thirteen codes are declared today with no datamodel entry. They are listed by name in the test and the length of that list is pinned, so it can only shrink and cannot be used to silence a new disagreement. Three of them are raised: VDI_IO_ERROR, REVERT_ONLY_ALLOWED_ON_SNAPSHOT and CLIENT_ERROR. Registering those needs a parameter list and a doc string for each, which is a separate change. The test collects its checks as a list and derives the exit code from it rather than relying on assert. Every check is generated by iterating one of the two lists, so a list that comes back empty yields no checks rather than failing ones; the test exits 2 rather than 0 in that case. It was made to fail on purpose five ways before being relied on. Delete ocaml/idl/errors.sh, which attempted the same comparison and was referenced by no dune file, makefile or script. It greps datamodel.ml rather than datamodel_errors.ml, matches the OCaml binding name rather than the error string, and exits zero whatever it finds. Signed-off-by: Gabriel Buica <danutgabriel.buica@citrix.com>
psafont
reviewed
Oct 6, 2026
| (* Pinning the length is what stops the baseline being used as an escape | ||
| hatch: discharging a code means deleting its line and lowering this | ||
| number, and no edit that adds one still passes. *) | ||
| let max_known_unregistered = 13 |
Member
There was a problem hiding this comment.
It's not clear to me why this is not Stringset.bindings known_unregistered_set
Contributor
|
Nothing in this PR is exclusive to MIG; could we merge it into master instead? |
This branch has not been deployed
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
3 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.
An API error is declared in two places: add_error in ocaml/xapi-consts/api_errors.ml makes it raisable, and the error function in ocaml/idl/datamodel_errors.ml declares its parameters and puts it in the generated SDK and the error reference. Nothing checked that both happened, so a code with only the first compiles, is raisable, and reaches a client as a string that appears in no reference.
Add test_error_registration to the tests stanza under ocaml/idl. It folds the two lists against each other and names any code present in one and absent from the other. Both are read at run time rather than parsed out of the source, because api_errors.ml builds the AUTH_ENABLE_FAILED_* family by concatenation and those codes have no string literal for a textual lint to find.
Thirteen codes are declared today with no datamodel entry. They are listed by name in the test and the length of that list is pinned, so it can only shrink and cannot be used to silence a new disagreement. Three of them are raised: VDI_IO_ERROR, REVERT_ONLY_ALLOWED_ON_SNAPSHOT and CLIENT_ERROR. Registering those needs a parameter list and a doc string for each, which is a separate change.
The test collects its checks as a list and derives the exit code from it rather than relying on assert. Every check is generated by iterating one of the two lists, so a list that comes back empty yields no checks rather than failing ones; the test exits 2 rather than 0 in that case. It was made to fail on purpose five ways before being relied on.
Delete ocaml/idl/errors.sh, which attempted the same comparison and was referenced by no dune file, makefile or script. It greps datamodel.ml rather than datamodel_errors.ml, matches the OCaml binding name rather than the error string, and exits zero whatever it finds.