fix: introduce AbsolutePathBuf and resolve relative paths in config.toml #7796
Conversation
| None if path.is_absolute() => { | ||
| Self::from_absolute_path(path).map_err(SerdeError::custom) | ||
| } | ||
| None => Err(SerdeError::custom( |
There was a problem hiding this comment.
I don't have a strong opinion. I thought since None if path.is_absolute() didn't panic, then it would be "gentler" for this not to panic.
There was a problem hiding this comment.
Come to think of it, it is likely important that None if path.is_absolute() does not panic, because if we want to use AbsolutePathBuf as an "input type" for app server (v2), we can have the requirement that callers pass in absolute paths.
There was a problem hiding this comment.
What I'd like to catch is when deserialization runs without the guard.
There was a problem hiding this comment.
But I don't think it's guaranteed to be a logic error. If V2 accepts AbsolutePathBuf over the wire and requires the client to specify an absolute path and the client sends a relative path, then app server should send back an error rather than crash, right?
| #[derive(Debug, Clone, PartialEq, Eq, Serialize)] | ||
| pub struct AbsolutePathBuf(PathBuf); | ||
|
|
||
| impl AbsolutePathBuf { |
There was a problem hiding this comment.
Thoughts on keeping this close to config and naming with something that includes config? PathRelaveToConfig :D
Do we want this type to spread to other parts of codebase?
There was a problem hiding this comment.
Given that I made this small crate just for AbsolutePathBuf, I think it should live near the type rather than in core/config in case we have other objects from which we want to deserialize it.
There was a problem hiding this comment.
Oh, sorry, my above comment was with respect to the guard.
There was a problem hiding this comment.
Do we want this type to spread to other parts of codebase?
Yes, I think that we do. It is helpful to enforce path absoluteness at construction and not have to check it all over the place later (particularly when you likely no longer have the base path handy, as was the case in otel_provider.rs).
|
@dtolnay not sure if this would be of interest to you, but it feels like |
This PR attempts to solve two problems by introducing a
AbsolutePathBuftype with a special deserializer:AbsolutePathBufattempts to be a generally useful abstraction, as it ensures, by constructing, that it represents a value that is an absolute, normalized path, which is a stronger guarantee than an arbitraryPathBuf.config.tomlthat can be either an absolute or relative path should be resolved against the folder containing theconfig.tomlin the relative path case. This PR makes this easy to support: the main cost is ensuringAbsolutePathBufGuardis used insidedeserialize_config_toml_with_base().While
AbsolutePathBufGuardmay seem slightly distasteful because it relies on thread-local storage, this seems much cleaner to me than using than my various experiments with https://docs.rs/serde/latest/serde/de/trait.DeserializeSeed.html. Further, since thedeserialize()method from theDeserializetrait is not async, we do not really have to worry about the deserialization work being spread across multiple threads in a way that would interfere withAbsolutePathBufGuard.To start, this PR introduces the use of
AbsolutePathBufinOtelTlsConfig. Note how this simplifiesotel_provider.rsbecause it no longer requiressettings.codex_hometo be threaded through. Furthermore, this sets us up better for a world where multipleconfig.tomlfiles from different folders could be loaded and then merged together, as the absolutifying of the paths must be done against the correct parent folder.