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

Allow to pass options to the underlying diagram libraries #827

Closed
ggrossetie opened this issue Jul 8, 2021 · 9 comments
Closed

Allow to pass options to the underlying diagram libraries #827

ggrossetie opened this issue Jul 8, 2021 · 9 comments
Labels
💡 proposal 🍩 enhancement New feature or request ☕ java Related to Java code

Comments

@ggrossetie
Copy link
Member

Some diagram libraries relies on options declared outside of the diagram definition.
These options are usually used to configure the output: margin, padding, size, style, font, layout...

For instance, when using Mermaid, we can define a configuration object to configure the font size, the layout direction or the maxium width and height: https://mermaid-js.github.io/mermaid/#/./Setup?id=configuration

The goal of this issue is to define a generic format to pass these options using the API.
Then, in a seconde phase, we will progressively add support for options on each diagram libraries (potentially disabling options that might be harmful in safe mode).

Query parameters

For GET requests, it makes sense to use query parameters:

GET /graphviz/svg/eNpLyUwvSizIUHBXqPZIzcnJ17ULzy_KSanlAgB1EAjQ?key=value

POST requests

POST /
{
  "diagram_source": "digraph G {Hello->World}",
  "diagram_type": "graphviz",
  "output_format": "svg",
  "diagram_options": {
    "key": "value"
  }
}

If the same option is defined as both a query parameter and as an attribute in the JSON body, I think the option defined in the JSON body should have an higher precedence.

HTTP headers

Since we can also send diagrams as plain text, we should also support HTTP headers:

POST /graphviz
Accept: image/svg+xml
Content-Type: text/plain
Kroki-Diagram-Options-Key: value

digraph G {
  Hello->World
}

In this case, it's important to use a prefix to disambiguate option names from other header names.
Again, if the same option is defined as both a query parameter and as a request header, I think the option defined as a request header should have an higher precedence.

Precedence order

  1. JSON body
  2. HTTP header
  3. Query parameter
@shah
Copy link

shah commented Jul 9, 2021

@Mogztter this is a great idea and I agree with the precedence order too!

@ggrossetie
Copy link
Member Author

ggrossetie commented Jul 24, 2021

It's important to note that HTTP headers are case-insensitive whereas query parameters and JSON keys are case-sensitive.

As a result, I think we should use all lower case when declaring options as query parameters or as attributes in the JSON body:

GET /graphviz/svg/eNpLyUwvSizIUHBXqPZIzcnJ17ULzy_KSanlAgB1EAjQ?viewkey=SignIn
{
  "diagram_options": {
    "viewkey": "SignIn"
  }
}

When declaring options as HTTP headers, the key will be converted to lower case. In other words, the following will be equivalent:

Kroki-Diagram-Options-ViewKey: SignIn
Kroki-Diagram-Options-viewKey: SignIn
kroki-diagram-options-viewkey: SignIn
KROKI-DIAGRAM-OPTIONS-VIEWKEY: SignIn

It will translate to { "viewkey": "SignIn" }.

As an alternative, we could use camel case instead of all lower case. Arguably, camel case is more readable but might be more error-prone?

@shah
Copy link

shah commented Jul 24, 2021

@Mogztter you could use the HTML5 web components naming rules:

  • Parameter must have at least one hyphen (e.g. view-key) and be all lowercase (kebab-case). This forces each variable to be namespaced, which is exactly what you want.
  • When there are hyphens and lowercase they be easily converted to camelCase, snake_case, etc. by the consumer/producer.

@ggrossetie
Copy link
Member Author

Parameter must have at least one hyphen (e.g. view-key) and be all lowercase (kebab-case). This forces each variable to be namespaced, which is exactly what you want.

kebab-case is fine but I have two small concerns:

  • we are already using snake_case in the JSON payload so it might be a bit confusing:

    {
      "diagram_options": {
        "view-key": "SignIn"
      }
    }
    

People might ask "Why view-key and not view_key?". Might also be true if we decide to use viewkey.

  • when using HTTP headers, it's harder to disambiguate the prefix from the key name:

    KROKI-DIAGRAM-OPTIONS-KEY-VIEW: SignIn
    

"The key name is VIEW or KEY-VIEW ?"

When there are hyphens and lowercase they be easily converted to camelCase, snake_case, etc. by the consumer/producer.

That's a good point but I don't know if we will need to convert from one casing to another.

I think I need to experiment to see what fits best.

@shah
Copy link

shah commented Jul 30, 2021

Good points @Mogztter but I actually like this ambiguation -- everything with kebab-case would be internal to a tool while underscores could be specific to Kroki.

{
  "diagram_options": {
    "view-key": "SignIn"
  }
}

And this is pretty readable to me:

KROKI-DIAGRAM-OPTIONS-KEY-VIEW: SignIn

ggrossetie added a commit to ggrossetie/kroki that referenced this issue Aug 2, 2021
@ggrossetie
Copy link
Member Author

Sneak peek with GraphViz:

graphviz-opts

ggrossetie added a commit to ggrossetie/kroki that referenced this issue Aug 3, 2021
@shah
Copy link

shah commented Aug 3, 2021

Very slick @Mogztter hopefully our friend from the Structurizr project will add to his project when this is released!

ggrossetie added a commit to ggrossetie/kroki that referenced this issue Aug 29, 2021
@sturtison
Copy link
Contributor

This is very nice, from the functionality down to the internal naming conventions of lower-kebab-case. Perhaps it can be used to nominate the theme used for PlantUML?

@ggrossetie
Copy link
Member Author

Perhaps it can be used to nominate the theme used for PlantUML?

Sure, could you please open a new issue?

Speaking of themes,I wonder if we should also include themes from https://plantumlstyler.netlify.app/.

Gray tone
graytone

Isaac
isaac

Deep sea
deepsea

Black and white
blackwhite

We should probably open another issue to discuss it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 proposal 🍩 enhancement New feature or request ☕ java Related to Java code
Projects
None yet
Development

No branches or pull requests

3 participants