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

Various breakages when formatting json heredocs #5930

Closed
dbaynard opened this issue Nov 7, 2023 · 8 comments · Fixed by caddyserver/website#352 or #6056
Closed

Various breakages when formatting json heredocs #5930

dbaynard opened this issue Nov 7, 2023 · 8 comments · Fixed by caddyserver/website#352 or #6056

Comments

@dbaynard
Copy link

dbaynard commented Nov 7, 2023

I've hit various issues, with 2.7.5 and earlier versions of caddy, when it comes to heredocs.

caddy fmt introduces spacing which means the resulting file is invalid.

See the caddyfile in https://github.com/dbaynard/caddy-fmt-heredoc-example — I recommend you step through the commits.

  1. Empty commit

  2. I created a sample, unformatted caddyfile, which works.

    	handle_path /never {
    		respond <<JSON
    		\{"response":"nope"\}
    		JSON 200
    	}
    	handle_path /json {
    		respond <<JSON
    		{"response":"nope"}
    		JSON 200
    	}
    

    The first appears to be returned as text/plain.

  3. I adapted it, and it fails to load

    	handle_path /never {
    		respond <<JSON \{
    		"response":"nope"\}
    		JSON 200
    	}
    	handle_path /json {
    		respond <<JSON 
    		{"response":"nope"}
    		JSON 200
    	}
    

    Note the space at the end of the JSON for /json.

    Error: adapting config using caddyfile: parsing caddyfile tokens for 'handle_path': unrecognized directive: response - are you sure your Caddyfile structure (nesting and braces) is correct?, at Caddyfile:18
  4. I removed the handle_path for /nope with escaped braces

Error: adapting config using caddyfile: parsing caddyfile tokens for 'handle_path': unrecognized directive: {"response":"nope"} - are you sure your Caddyfile structure (nesting and braces) is correct?, at Caddyfile:18
  1. I removed the space after the heredoc market, and it's fine, again.

I tried with various different sigils, and saw no difference. In the repo, I have an example with a placeholder (that works correctly). And I have only tried the json formats.

Incidentally, the correct version of this would be helpful as an example.

@francislavoie
Copy link
Member

Yeah, unfortunately caddy fmt doesn't handle heredocs at all right now. The problem is the Caddyfile lexer is completely separate code from the formatter. We'd have to re-implement heredoc lexing in the formatter as well, which is tedious. I want to rewrite the lexer to make it retain whitespace so that it can also be used for formatting, but that's a significant undertaking.

For now, unfortunately you'll need to avoid using caddy fmt if you're using heredocs.

@dbaynard
Copy link
Author

dbaynard commented Nov 7, 2023

Thanks for confirming. I'll skim the documentation — there may be a tweak I can make, and then I can close this.

@francislavoie
Copy link
Member

Oh I missed this in your post:

The first appears to be returned as text/plain.

Yeah the heredoc tag is purely textual and doesn't affect mime parsing because it's just for the Caddyfile. You'll need to add a header Content-Type directive alongside the respond to send the right content type.

@dbaynard
Copy link
Author

dbaynard commented Nov 7, 2023

Thanks — yes, I did know about that. As the docs on the respond directive says,

If the body is non-empty, this directive sets the Content-Type header if it is not already set. The default value is text/plain; utf-8 unless the body is a valid JSON object or array, in which case it is set to application/json. For all other types of content, set the proper Content-Type explicitly using the header directive.

I thought this automatic detection was working correctly in the example with the non-escaped braces. Where I escaped the braces, I did need to add the header, contrary to the impression from the docs.

I only escaped the braces because in my example I had placeholders, and I initially thought there was an issue parsing the placeholder from the JSON (I reported a minimal repro; there is an example with a placeholder in the repro repo). Then I discovered caddy fmt was breaking the heredoc.

@francislavoie
Copy link
Member

Ah I see, yeah it's not necessary to escape the JSON here. So your second handle-path should be what you use, and just avoid caddy fmt for now.

dbaynard added a commit to dbaynard/nixpkgs that referenced this issue Nov 7, 2023
As explained in
caddyserver/caddy#5930 (comment),
`caddy fmt` doesn’t handle heredocs. The existing module always runs
`caddy fmt` on native builds, and thus for a configuration containing a
heredoc, the service fails at runtime.

This change is simple, and backwards compatible. Alternatives I
considered include only using the output of `caddy fmt` if `caddy adapt`
also succeeds, removing the formatting and adding an option defaulting to false.
Adding an option that defaults to true allows users to disable `caddy fmt`
for other reasons, thoguh.
dbaynard added a commit to dbaynard/caddy-website that referenced this issue Nov 7, 2023
The lexer and formatter are separate, and the additional work is a
significant undertaking.

caddyserver/caddy#5930 (comment)
@francislavoie
Copy link
Member

francislavoie commented Jan 22, 2024

FYI @dbaynard, thanks to @bbaa-bbaa this should now be properly fixed 🎉

Also FYI @matthewpi since the vscode extension will be improved by this 😅

@dbaynard
Copy link
Author

dbaynard commented Jan 22, 2024

This is great — thanks @bbaa-bbaa

caddyserver/website#352 will need a change — I can note the first caddy version for which this feature works?

@francislavoie
Copy link
Member

We'll just remove the note from caddyserver/website#352 when we release the next version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants