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

Bug/5123 allow attribute names to be escaped on er diagram #5138

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

ejscribner
Copy link

📑 Summary

Enables special characters within backticked strings for the attributeName in an ER Diagram.

Resolves #5123

📏 Design Decisions

To allow more flexibility in an attributeName, I've used backticks to escape all special characters that a user might need to put there.

This approach:

  1. Doesn't break the current (stricter) parsing functionality as it just adds a new BACKTICK_STRING token
  2. Allows for much increased flexibility if users need it

📋 Tasks

Make sure you

Copy link

netlify bot commented Dec 12, 2023

Deploy Preview for mermaid-js ready!

Name Link
🔨 Latest commit 779178c
🔍 Latest deploy log https://app.netlify.com/sites/mermaid-js/deploys/65aeca7ff1743e0008133bf6
😎 Deploy Preview https://deploy-preview-5138--mermaid-js.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added the Type: Bug / Error Something isn't working or is incorrect label Dec 12, 2023
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7b62466) 43.04% compared to head (779178c) 79.35%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##           develop    #5138       +/-   ##
============================================
+ Coverage    43.04%   79.35%   +36.31%     
============================================
  Files           23      167      +144     
  Lines         5018    13873     +8855     
  Branches        21      707      +686     
============================================
+ Hits          2160    11009     +8849     
+ Misses        2857     2711      -146     
- Partials         1      153      +152     
Flag Coverage Δ
e2e 85.16% <ø> (?)
unit 43.04% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 163 files with indirect coverage changes

Copy link
Contributor

@nirname nirname left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Some minor changes and this will be ok

packages/mermaid/src/diagrams/er/parser/erDiagram.jison Outdated Show resolved Hide resolved
@@ -26,6 +26,7 @@ accDescr\s*"{"\s* { this.begin("acc_descr_multili
<block>\b((?:PK)|(?:FK)|(?:UK))\b return 'ATTRIBUTE_KEY'
<block>(.*?)[~](.*?)*[~] return 'ATTRIBUTE_WORD';
<block>[\*A-Za-z_][A-Za-z0-9\-_\[\]\(\)]* return 'ATTRIBUTE_WORD'
<block>\`[^`\n]*\` return 'BACKTICK_STRING';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to add backtick within attribute name as well? I am asking because we are trying to allow basically any characters within attribute name with this PR. Minor

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding one more backtick in the name causes a parse error, and adding a second one breaks it into two attributes. For example:

String `geo.`acc`uracy` 

becomes:
Screenshot 2024-01-22 at 12 10 04 PM

I'm not sure if there is a way around this behavior, other than maybe changing the escape characters from backticks to something else. Let me know if you have suggestions here 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should be escaped then. Common practice is to use escapement character, like \ (\\ and \` respectively for \ and `) or duplicating characters, like "hello""quotes" (csv uses that approach), or using another character as wrapper (like double backtick for markdown)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure which approach to pick up, they all have some flaws but it's better to think about such a feature beforehand because it is a real pain to introduce something like this later without braking backwards compatibility

packages/mermaid/src/diagrams/er/parser/erDiagram.spec.js Outdated Show resolved Hide resolved
@ejscribner
Copy link
Author

ejscribner commented Jan 22, 2024

Thanks for the review @nirname, fixed the easy ones. Let me know if you have any ideas or thoughts on the backticks in attribute name one.

@ejscribner ejscribner requested a review from nirname January 22, 2024 20:13
@nirname
Copy link
Contributor

nirname commented Jan 23, 2024

Please, do not forget to add some documentation describing this, otherwise people wont be aware of that feature.

@nirname
Copy link
Contributor

nirname commented May 29, 2024

Sorry for the long response.

@ejscribner please add some documentation describing this feature

@sidharthv96 @aloisklink what do you think on escapement syntax for backtics?

@nirname
Copy link
Contributor

nirname commented May 29, 2024

There are 2 main options

Escape using `` double backtick

source outcome
backticked string with `` in it backticked string with ` in it
backticked string with \ in it backticked string with \ in it
backticked string with in it` error

Escape using \ backslash

source outcome
backticked string with \ in it` backticked string with ` in it
backticked string with \\ in it backticked string with \ in it
backticked string with in it` error

Copy link
Contributor

@nirname nirname left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is OK. We need some documentation covering this feature to continue

@mjpieters
Copy link

I would love to see this land.

We use https://github.com/k1LoW/tbls/ to generate database documentation including Mermaid ER diagrams. However, I currently have to disable generating Mermaid ERDs for a specific database view that uses some special characters in the column names (<= and > respectively), as these can't be escaped with the current mermaid syntax.

Is there any way I can help pull this over the line?

@nirname
Copy link
Contributor

nirname commented Jun 19, 2024

@mjpieters Yes, you definitely can. This is half-ready. Whether @ejscribner is going to continue on this or not, you can always take what has already been done, open new PR to the issue #5123 and we can continue over there.

Anyway you can help with a piece of advise so we could come up with convenient format for escaping, and I could complete the feature myself. It's up to you, any help appreciated.

@nirname nirname self-assigned this Jun 19, 2024
@nirname
Copy link
Contributor

nirname commented Jun 19, 2024

I'll assign myself to this PR so it would be easier to find this among other issues. Doesn't necessarily mean we continue this very branch.

@stevemuench
Copy link

Also very interested in seeing this feature land. Oracle table column names frequently have a dollar-sign in them and would love to be able to render data types like varchar2(32) with parens in the name via appropriate escaping.

@mjpieters
Copy link

mjpieters commented Jun 25, 2024

There are 2 main options
[...]

There is another option: accept character references, so &grave; or &#60;, or something based on these. See below.

Normally, I'd say you should pick an option that is consistent with the syntax used by other mermaid diagrams. Unfortunately, there is no consistency here:

GIven that mermaid is specifically designed to be embedded inside Markdown documents, I'd go with the backslash to escape backticks (and backslashes, because you need to support escaping a backslash too).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug / Error Something isn't working or is incorrect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow attribute names to be escaped on ER diagram
4 participants