Make keyword converters type-safe and fix related issues - #1107
Merged
Conversation
The failover keyword of the fs.sgcp_nfs_cg driver names a "boolean" converter, which is not registered. converters.Lookup() panics on an unknown name, so starting an object with such a resource crashes in configureResource(), through EvalKeywordAs() and xconfig.convert(). The registered name is "bool".
A keyword used to name its converter with a string, resolved at
evaluation time by converters.Lookup(), which panics when no converter
is registered under that name. A typo in a keyword definition was thus
undetectable until an object using that keyword was started.
Change keywords.Keyword.Converter to a converters.Converter, and export
a singleton for each converter, so a keyword definition references the
converter instead of naming it. A typo is now a compilation error. A nil
Converter means no conversion, which the empty name used to mean.
converters.Lookup() is replaced by:
* converters.Get(), which reports whether the name is known instead of
panicking. It is only needed at the daemon API boundary, where the
converter name of a keyword definition comes from a peer daemon that
may run a different version, and so may name a converter this
version does not know. Such a keyword now degrades to no conversion
instead of crashing the client.
* converters.Name(), which returns the name a converter is registered
as, to feed the string field of the API keyword definition item.
The API schema is unchanged: KeywordDefinitionItem.Converter remains a
string.
The nodes and peers converters, which live in core/xconfig because they
depend on the node selector, get the same treatment through
xconfig.NodesConverter and xconfig.PeersConverter.
Now that a keyword references its converter instead of naming it, the compiler rejects an unknown converter. Two mistakes remain possible: a converter implemented but never registered, which would break the name exposed by the API, and a keyword default value the keyword converter refuses. Add a test walking the node keyword store, the keyword store of every object kind and the keyword store of every registered driver, and asserting both. The per-driver stores are walked because a driver manifest declaring no kind is not reachable from object.KeywordStoreWithDrivers().
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
This pull request enhances type safety for keyword converters by transitioning from string-based naming to direct type references. Previously, string-based errors could remain undetected until evaluation, causing runtime failures. Additionally, a sanity test is added to validate converter registration and correctness.
Key Changes
keywords.Keyword.Converterto useconverters.Converterdirectly, replacing string-based definitions.converters.Get()to safely retrieve converters and prevent panics on unknown names.fs.sgcp_nfs_cgdriver ("boolean"replaced with"bool"to fix runtime errors).Notes
These changes do not affect the external API schema, maintaining compatibility with existing integrations.