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(Diagram): fix persisted data due to db not being cleared before parsing #3310

Merged
merged 2 commits into from Aug 11, 2022

Conversation

hrgui
Copy link
Contributor

@hrgui hrgui commented Aug 10, 2022

📑 Summary

Data was being persisted to subsequent newer mermaid Diagrams called via new.

Resolves #3305

📏 Design Decisions

I noticed that the this.db was not cleared after each call to new Diagram(...). Since the db for each diagram type is a singleton, we need to to call clear.

Otherwise, subsequent calls of the same type will persist the data to the next diagram created.

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added unit/e2e tests (if appropriate)
  • 🔖 targeted develop branch

@github-actions github-actions bot added the Discussion Discussion which may lead to separate issues or PRs label Aug 10, 2022
sequenceDiagram
Alice->Bob:Hello Bob, how are you?
Bob-->Alice: I am good thanks!`);
expect(diagram1.db.getMessages()).toMatchInlineSnapshot(`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If preferred, we can change this to a normal assertion, but I found an inline snapshot to be the fastest for understanding what is going on.

This is how it looks likes when this.db.clear() is not called:

describe('more than one sequence diagram', () => {
  it('should not have duplicated messages', () => {
    const diagram1 = new Diagram(`
        sequenceDiagram
        Alice->Bob:Hello Bob, how are you?
        Bob-->Alice: I am good thanks!`);
    expect(diagram1.db.getMessages()).toMatchInlineSnapshot(`
      Array [
        Object {
          "from": "Alice",
          "message": "Hello Bob, how are you?",
          "to": "Bob",
          "type": 5,
          "wrap": false,
        },
        Object {
          "from": "Bob",
          "message": "I am good thanks!",
          "to": "Alice",
          "type": 6,
          "wrap": false,
        },
      ]
    `);
    const diagram2 = new Diagram(`
        sequenceDiagram
        Alice->Bob:Hello Bob, how are you?
        Bob-->Alice: I am good thanks!`);

    expect(diagram2.db.getMessages()).toMatchInlineSnapshot(`
      Array [
        Object {
          "from": "Alice",
          "message": "Hello Bob, how are you?",
          "to": "Bob",
          "type": 5,
          "wrap": false,
        },
        Object {
          "from": "Bob",
          "message": "I am good thanks!",
          "to": "Alice",
          "type": 6,
          "wrap": false,
        },
        Object {
          "from": "Alice",
          "message": "Hello Bob, how are you?",
          "to": "Bob",
          "type": 5,
          "wrap": false,
        },
        Object {
          "from": "Bob",
          "message": "I am good thanks!",
          "to": "Alice",
          "type": 6,
          "wrap": false,
        },
      ]
    `);

    // Add John actor
    const diagram3 = new Diagram(`
        sequenceDiagram
        Alice->John:Hello John, how are you?
        John-->Alice: I am good thanks!`);

    expect(diagram3.db.getMessages()).toMatchInlineSnapshot(`
      Array [
        Object {
          "from": "Alice",
          "message": "Hello Bob, how are you?",
          "to": "Bob",
          "type": 5,
          "wrap": false,
        },
        Object {
          "from": "Bob",
          "message": "I am good thanks!",
          "to": "Alice",
          "type": 6,
          "wrap": false,
        },
        Object {
          "from": "Alice",
          "message": "Hello Bob, how are you?",
          "to": "Bob",
          "type": 5,
          "wrap": false,
        },
        Object {
          "from": "Bob",
          "message": "I am good thanks!",
          "to": "Alice",
          "type": 6,
          "wrap": false,
        },
        Object {
          "from": "Alice",
          "message": "Hello John, how are you?",
          "to": "John",
          "type": 5,
          "wrap": false,
        },
        Object {
          "from": "John",
          "message": "I am good thanks!",
          "to": "Alice",
          "type": 6,
          "wrap": false,
        },
      ]
    `);
  });
});

Notice that we expect 2 messages per each diagram, but in the last diagram we have 6 messages.

@hrgui hrgui marked this pull request as ready for review August 10, 2022 04:55
src/Diagram.js Outdated Show resolved Hide resolved
@hrgui
Copy link
Contributor Author

hrgui commented Aug 10, 2022

For the E2E test, I did notice there is a test that tests the use case I was trying to tackle.

https://github.com/mermaid-js/mermaid/blob/develop/cypress/integration/other/configuration.spec.js#L108-L117

and

https://github.com/mermaid-js/mermaid/blob/develop/cypress/platform/regression/issue-1874.html#L15-L28

BEFORE the fix
Screen Shot 2022-08-09 at 10 33 02 PM

AFTER the fix
Screen Shot 2022-08-09 at 10 33 49 PM

@knsv
Copy link
Collaborator

knsv commented Aug 11, 2022

Good catch! Much appreciated!

Copy link
Collaborator

@knsv knsv left a comment

Choose a reason for hiding this comment

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

Looks good!

@knsv knsv merged commit cb4b60e into mermaid-js:develop Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Discussion which may lead to separate issues or PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

subsequent sequence diagrams contain previous diagrams (new to 9.1.4)
2 participants