Repository navigation
Fix import chaining for custom header support - #618
Merged
Merged
Conversation
Fixes dhall-lang/dhall-json#60 In the above issue, a remote import specified custom headers using a relative path: ``` $ curl https://gist.githubusercontent.com/reactormonk/a8192fe784608f1f7515d5583c448095/raw/d46f5b9aea9eb56c4bdd92d187936ee87e9f4cf2/local.dhall let test = https://gist.githubusercontent.com/reactormonk/a8192fe784608f1f7515d5583c448095/raw/e8e5b14c2afe9acc342fe111bcf6e09b90dced9c/test.dhall using ./headers in test ``` But import chaining was not working correctly for the `using` clause of the import, meaning that before this change the `./headers` import was not anchored to the parent import. Using the `</>` judgment from the standard, the previous behavior was: ``` http://example0.com/foo </> http://example1.com/bar using ./baz = http://example1.com/bar using ./baz ``` ... when the behavior should have been: ``` http://example0.com/foo </> http://example1.com/bar using ./baz = http://example1.com/bar using http://example0.com/baz ``` Note that custom header support has not yet been fully standardized, but the latter version is what the correct behavior should be once it is standardized. The reason the former version is problematic is because it does not behave correctly in the presence of header forwarding. If `http://example1.com/bar` imports any remote expressions of its own, it will use the relative import `./baz` to customize the headers for downstream imports from the same domain. However, because this relative import is not anchored to the parent import, it will change for each downstream import. For example, suppose that the contents of `http://example1.com/bar using ./baz` are: ``` http://example1.com/baz ``` Import chaining that after its parent import would give: ``` http://example1.com/bar using ./baz </> http://example1.com/baz = http://example1.com/baz using ./baz ``` ... which introduces an inadvertent import cycle because now `./baz` will resolve to `http://example1.com/baz` instead of `http://example0.com/baz`. An import cycle similar to this one caused the bug described by the linked issue. This change fixes import chaining to correctly anchor custom header imports to the parent import and this fix resolves the above issue. This also includes a small change to fix canonicalization to also canonicalize the custom header import, although this is unrelated to the above bug.
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
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.
Fixes dhall-lang/dhall-json#60
In the above issue, a remote import specified custom headers using a
relative path:
But import chaining was not working correctly for the
usingclause of theimport, meaning that before this change the
./headersimport was notanchored to the parent import.
Using the
</>judgment from the standard, the previous behavior was:... when the behavior should have been:
Note that custom header support has not yet been fully standardized, but the
latter version is what the correct behavior should be once it is standardized.
The reason the former version is problematic is because it does not behave
correctly in the presence of header forwarding. If
http://example1.com/barimports any remote expressions of its own, it will use the relative import
./bazto customize the headers for downstream imports from the same domain.However, because this relative import is not anchored to the parent import, it
will change for each downstream import.
For example, suppose that the contents of
http://example1.com/bar using ./bazare:Import chaining that after its parent import would give:
... which introduces an inadvertent import cycle because now
./bazwillresolve to
http://example1.com/bazinstead ofhttp://example0.com/baz. Animport cycle similar to this one caused the bug described by the linked issue.
This change fixes import chaining to correctly anchor custom header imports to
the parent import and this fix resolves the above issue.
This also includes a small change to fix canonicalization to also canonicalize
the custom header import, although this is unrelated to the above bug.