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

Database Documentation extension: generates mermaid diagrams for databases, schemas, and tables #23409

Closed
wants to merge 0 commits into from

Conversation

laurennathan
Copy link

Starting code for the database documentation extension. Code generates mermaid diagrams for databases, schemas and tables

@@ -0,0 +1,18 @@
{
"root": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this and use the same eslintrc we have in other extensions (except with different project path) so we get all the default eslint stuff for extensions automatically

@@ -0,0 +1,9 @@
# Change Log
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this for now - we don't usually have a changelog for extensions

extensions/database-documentation/README.md Outdated Show resolved Hide resolved
* Licensed under the Source EULA. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

var https = require('https');
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this, not needed for extensions living in the ADS repo. The refs.d.ts will point directly to them instead.

extensions/database-documentation/package-lock.json Outdated Show resolved Hide resolved
// The commandId parameter must match the command field in package.json
context.subscriptions.push(vscode.commands.registerCommand('database-documentation.generateDocumentation', async (context: azdata.ObjectExplorerContext) => {
// The code you place here will be executed every time your command is executed
vscode.window.showInformationMessage("Generating Documentation... ");
Copy link
Contributor

Choose a reason for hiding this comment

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

This (and all other user-visible messages) should be localized (see other extensions for an example using the localize function)

Comment on lines 28 to 32
// Use the console to output diagnostic information (console.log) and errors (console.error)
// This line of code will only be executed once when your extension is activated
console.log('Congratulations, your extension "database-documentation" is now active!');


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Use the console to output diagnostic information (console.log) and errors (console.error)
// This line of code will only be executed once when your extension is activated
console.log('Congratulations, your extension "database-documentation" is now active!');

import * as vscode from 'vscode';
// import * as myExtension from '../../extension';

suite('Extension Test Suite', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these test files for now if you aren't going to add any real tests to start with

@@ -0,0 +1,37 @@
# Welcome to your Azure Data Studio Extension
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this

@@ -0,0 +1,20 @@
```mermaid
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this for?

@Charles-Gagnon
Copy link
Contributor

There's some additional files that need to be updated as well, see #23260 for an example

"project": "./extensions/database-documentation/tsconfig.json"
},
"rules": {
// Disabled until the issues can be fixed
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't disable this - all new extensions should be fixing issues right away instead

"Other"
],
"activationEvents": [
"*"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make an issue to track finding a better activation event for this - we should have as few * activations as possible.

"mermaid": "^10.1.0",
"mocha": "^9.1.0",
"typescript": "^4.3.5",
"vscode-nls": "^4.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

These are already in dependencies section


const localize = nls.loadMessageBundle();

export async function generateMarkdown(context: azdata.ObjectExplorerContext): Promise<string> {
Copy link
Contributor

Choose a reason for hiding this comment

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

@aasimkhan30 Please make sure someone is reviewing this code - I won't have time to spend on this at the moment

(and ideally this is actually done in a separate PR, but I'll leave that up to you two)

else {
const queryProvider = azdata.dataprotocol.getProvider<azdata.QueryProvider>("MSSQL", azdata.DataProviderType.QueryProvider);
const connectionUri = await azdata.connection.getUriForConnection(connection.connectionId);
let query = `SELECT TABLE_NAME FROM [${databaseName}].INFORMATION_SCHEMA.TABLES WHERE TABLE_TYPE = 'BASE TABLE'`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure you're escaping ] and ' whenever you manual create T-SQL statements like this to prevent SQL injection type issues. (easy way is to test with a DB that has both those characters in its name)

https://learn.microsoft.com/en-us/sql/relational-databases/security/sql-injection?view=sql-server-ver16

Copy link
Contributor

Choose a reason for hiding this comment

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

make sure this comment is addressed.

Comment on lines 13 to 17
"strict": false,
"noImplicitAny": false,
"noImplicitReturns": false,
"noUnusedLocals": false,
"strictNullChecks": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove these and fix any issues - all new extensions shouldn't be disabling these

Suggested change
"strict": false,
"noImplicitAny": false,
"noImplicitReturns": false,
"noUnusedLocals": false,
"strictNullChecks": false,

@kburtram
Copy link
Member

@laurennathan could you please rebase this PR to merge from one "feature branch" to another? We can take into main at the end of your project. This should allow you to create multiple PRs for changes incrementally without having to get the code into a production state upfront.

let databaseName = context.connectionProfile.databaseName;

let tables: string[];
if (context.nodeInfo.nodeType === 'Table') {
Copy link
Member

Choose a reason for hiding this comment

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

are we also going to support views, sprocs, or other schema objects?

tables = (await queryProvider.runQueryAndReturn(connectionUri, query)).rows.map(row => row[0].displayValue);
}

let diagram = '```mermaid\nclassDiagram\n';
Copy link
Member

Choose a reason for hiding this comment

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

have you tried using an erDiagram? I think that is the more appropriate chart for a db schema and wonder if there is any differences built in, such as displaying cardinalities on relationships, or something like that?

Copy link
Member

@kburtram kburtram left a comment

Choose a reason for hiding this comment

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

Please rebase off a "feature branch" off main and let's iterate there for a few more weeks before going into main branch. This will enable you to send more frequent PRs for smaller bits of functionality through the project.

out/test
src
.gitignore
vsc-extension-quickstart.md
Copy link
Contributor

Choose a reason for hiding this comment

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

for this file and the .gitignore file, please only leave the entries that are relevant to your extension.

@@ -0,0 +1,5 @@
{
"displayName": "Database Documentation and Diagrams",
"description": "Generate Documentation and Diagrams for Database Schemas",
Copy link
Contributor

Choose a reason for hiding this comment

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

the description doesn't seem to be accurate.

  1. Mention this is only for Microsoft SQL Server and SQL Database.
  2. IIUC, this extension works with databases, schemas, tables and views.

import * as azdata from 'azdata'
import * as vscode from 'vscode';
// Language localization features
import * as nls from 'vscode-nls';
Copy link
Contributor

Choose a reason for hiding this comment

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

these comments are not needed

let query = `SELECT TABLE_NAME FROM [${databaseName}].INFORMATION_SCHEMA.TABLES WHERE TABLE_TYPE = 'BASE TABLE'`;

if (context.nodeInfo.nodeType === 'Schema') {
query += ` AND TABLE_SCHEMA = '${context.nodeInfo.metadata.name}'`;
Copy link
Contributor

Choose a reason for hiding this comment

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

potential sql injection risk.

@laurennathan laurennathan force-pushed the laurennathan/database-documentation-PR branch from a02d845 to 5f7a182 Compare June 26, 2023 21:01
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 this pull request may close these issues.

5 participants