Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix import chaining for custom header support #618

Merged
merged 2 commits into from
Oct 4, 2018

Conversation

Gabriella439
Copy link
Collaborator

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.

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.
@Gabriella439 Gabriella439 merged commit 3b3c14b into master Oct 4, 2018
@Gabriella439 Gabriella439 deleted the gabriel/fix_chain branch October 4, 2018 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OOM killed when using headers with file with relative import
1 participant