KTOR-4206 Wrap Typesafe exceptions in HOCON config - #5915
Open
Marko Cevrljakovic (MARKOCEVRLJAKOVIC) wants to merge 2 commits into
Open
Marko Cevrljakovic (MARKOCEVRLJAKOVIC) wants to merge 2 commits into
Marko Cevrljakovic (MARKOCEVRLJAKOVIC) wants to merge 2 commits into
Conversation
HoconApplicationConfig.config() and configList() called Typesafe's getConfig/getConfigList directly, so a missing or mistyped path leaked com.typesafe.config.ConfigException to callers instead of Ktor's ApplicationConfigurationException. property() already follows the ApplicationConfig contract; these two methods did not. Route both methods through a helper that translates the exceptions and keeps the original one as the cause: - ConfigException.Missing is reported as "Path <path> not found." - Any other ConfigException (WrongType, BadPath, NotResolved, ...) is reported as "Failed to read path <path>: <original message>" A single generic catch is used for the non-Missing cases so that no Typesafe exception, including future subtypes, can leak through.
Pantus Oleh (zibet27)
self-requested a review
October 1, 2026 08:32
Pantus Oleh (zibet27)
requested changes
Oct 1, 2026
Pantus Oleh (zibet27)
left a comment
Collaborator
There was a problem hiding this comment.
Hi Marko Cevrljakovic (@MARKOCEVRLJAKOVIC),
Thank you! The code looks good to me.
I think we should also wrap exceptions in HoconApplicationConfigValue and add @throws to its KDoc; other implementations already throw that.
One exception, YamlNodeConfigValue.getMap throwing IllegalStateException which rather typo.
Will you do that?
HoconApplicationConfigValue called Typesafe directly, so getString, getList, getMap, getAs and type leaked ConfigException. They now go through wrapConfigException and throw ApplicationConfigurationException. property() and propertyOrNull() also wrap hasPath(), which throws ConfigException for an invalid path or an unresolved substitution. Align the other implementations with the documented contract: - YamlNodeConfigValue.getMap throws ApplicationConfigurationException instead of IllegalStateException. - MapApplicationConfigValue.getString and getList throw ApplicationConfigurationException instead of NullPointerException for a missing key. Add @throws to the KDoc of ApplicationConfigValue.getString, getList and getMap, and shorten the test names. Update HoconDecoderTest "invalid types": getAs() now throws ApplicationConfigurationException with ConfigException.WrongType as the cause, instead of leaking WrongType directly.
Contributor
Author
|
Hi Pantus Oleh (@zibet27) , thanks for the review! I've pushed the changes, please let me know if everything looks good now. Additionally I had to update an existing test in HoconDecoderTest. It expected getAs() to throw ConfigException.WrongType directly, which is the leak this PR fixes. I also changed MapApplicationConfigValue to throw ApplicationConfigurationException instead of NullPointerException, so it matches the new KDoc. Let me know if that's okay. |
Marko Cevrljakovic (MARKOCEVRLJAKOVIC)
requested a review
from Pantus Oleh (zibet27)
October 1, 2026 13:52
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
2 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.
Subsystem
Server, Config
Motivation
Fixes KTOR-4206
HoconApplicationConfig.config()andconfigList()call Typesafe Config directly, so a missing or invalid path throwscom.typesafe.config.ConfigExceptioninstead ofApplicationConfigurationException. This is inconsistent withproperty()in the same class and with the YAML backend, which both throwApplicationConfigurationException.Solution
config()andconfigList()now route through a private helper,wrapConfigException, which translates Typesafe exceptions intoApplicationConfigurationExceptionwhile preserving the original exception ascause:ConfigException.Missing-"Path $path not found."(the most common case, with a clear message).ConfigException-"Failed to read path $path: <original message>".I deliberately used a generic
catch (cause: ConfigException)for everything exceptMissing. This coversWrongType,BadPath,NotResolved, and any future subtypes, guaranteeing that no Typesafe exception can leak through these methods. The original message is preserved in the text and the typed exception remains accessible viacause, so no diagnostic information is lost.An alternative would be catching each subtype separately (
WrongType,BadPath,NotResolved, etc.) with adjusted messages. That offers slightly more specific error messages, but it requires more code and unlisted subtypes could still leak. I'm happy to switch to per-subtype handling if maintainers prefer it.Testing
HoconConfigTest:
config()for top-level, nested, and relative-to-sub-config paths, andconfigList(). Each asserts the message and thatcauseisConfigException.Missing.config()on a scalar andconfigList()on an object. Each asserts thatcauseisConfigException.WrongType."","ktor."and"ktor..deployment"for both methods, asserting the"Failed to read path ..."message.config()on a non-resolved substitution, asserting thatcauseisConfigException.NotResolved.MergedApplicationConfigTest:
config()properly wraps exceptions when reading missing paths from a merged/fallback configuration setup.Targeting release/3.x as a bug fix. Happy to retarget to main if needed.