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

Switch to POST if the URI is too long #42

Closed
ggrossetie opened this issue Apr 17, 2020 · 10 comments · Fixed by #77
Closed

Switch to POST if the URI is too long #42

ggrossetie opened this issue Apr 17, 2020 · 10 comments · Fixed by #77

Comments

@ggrossetie
Copy link
Member

When using very large Vega specification file (when data is embed in the file), the public instance of Kroki can return the following error "414 Request-URI Too Large nginx/1.14.0 (Ubuntu)".

I think we should use a POST request to avoid this error if the URI is longer than 4096.

See: yuzutech/kroki-go#11

@eshepelyuk
Copy link

@Mogztter why not use POST by default if kroki server supports this ?

@ggrossetie
Copy link
Member Author

My main concern is corporate proxy/firewall. I believe that it's more likely that a POST request will be blocked than a GET request but maybe that's not a good enough reason.

In one of my previous job the corporate proxy was a nightmare so I might be biased 😂

@eshepelyuk
Copy link

eshepelyuk commented Apr 25, 2020

Hey, lets introduce an attribute for this ?
Called kroki-http-method - with following values

  • get - means always use GET
  • post - means always use POST
  • adaptive - try to use GET, unless query string length is less than 4096 bytes, then fall back to POST

For instance, we wilk use local kroki and we would like to force POST always

@ggrossetie
Copy link
Member Author

That's a good idea 👍

@ggrossetie
Copy link
Member Author

There are several challenges we need to address.

When using the option "inline" we delegate all the work to Asciidoctor.js (ie. read the image data from the specified URI and generate a data URI). In this case, Asciidoctor.js will use a GET request to read the image data from the specified URI... 😞

Another issue is that we are currently using the Virtual File System to read the image data from the specified URI but if we want to use a POST request then we need to explicitly use an HTTP client.

@ggrossetie
Copy link
Member Author

It's actually possible to set the target as a data-uri: asciidoctor/asciidoctor#3657 (comment) 🎉

@eshepelyuk
Copy link

@Mogztter although issue is closed, the README is not updated :((

@ggrossetie
Copy link
Member Author

Indeed it was automatically closed when I merged the pull request, reopening 😉

@ggrossetie
Copy link
Member Author

README updated!
I think we can close now? @eshepelyuk any objection?

@eshepelyuk
Copy link

@Mogztter plz close

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 a pull request may close this issue.

2 participants