Better locations for URL strings #6031
Conversation
# Conflicts: # apollo-federation/src/sources/connect/validation/http/url.rs
dylan-apollo
requested review from
nicholascioli,
a team,
dariuszkuc,
sachindshinde,
goto-bus-stop,
SimonSapin,
lrlna,
TylerBloom and
duckki
as code owners
| @@ -3984,6 +3985,12 @@ dependencies = [ | |||
| "vcpkg", | |||
| ] | |||
|
|
|||
| [[package]] | |||
| name = "line-col" | |||
There was a problem hiding this comment.
I'm a little wary of an unstable dependency that hasn't been updated in four years - how much value is this providing? It looks like a very small amount of code.
| /// Get the raw string value of this GraphQL string literal | ||
| /// | ||
| /// Returns `None` if the value was not a string literal or if location data is messed up | ||
| pub fn new(value: &'schema Node<Value>, sources: &'schema SourceMap) -> Option<Self> { |
There was a problem hiding this comment.
Nitpick: idiomatically, this seems more like a Result than an Option, since the "messed up" cases would be an error. All callers also use ok_or... to convert to a Result.
|
|
||
| pub(super) struct SchemaInfo<'schema> { | ||
| pub(crate) schema: &'schema Schema, | ||
| pub(crate) lookup: LineColLookup<'schema>, |
There was a problem hiding this comment.
I'm wary of including dependencies in public API (even pub(crate)) as it makes it more difficult to remove or update the dependency. Especially in this case, where there is really only a single method on the dependency type, this would be very easy to just wrap with a method on this struct rather than exposing the LineColLookup directly. Then pass around SchemaInfo instead of the lookup field.
| let start_offset = self.offset + substring_location.start; | ||
| let end_offset = self.offset + substring_location.end; | ||
|
|
||
| let (line, column) = lookup.get(start_offset); |
There was a problem hiding this comment.
This call panics if the offset is incorrect, which is very worrisome. You need to either document this in a # Panics rustdoc on this method, or you could validate the offsets within this method with a fallible Result. But really this is making me question this dependency even more.
| &self, | ||
| substring_location: Range<usize>, | ||
| lookup: &LineColLookup, | ||
| ) -> Option<Range<LineColumn>> { |
There was a problem hiding this comment.
This appears to be infallible, why the Option?
This fixes error locations within multiline/triple-quote URL strings, and sets up utilities so we can do the same within JSONSelection!
There's also a significant refactor to reduce the amount of things we pass between functions, since I just added one more thing ™️