sql_table: support multiple constraints via arrays

[satoqz]

Description

This PR adds a primary_key_foreign_key option to the constraint field of the sql_table, rendered as "PK FK". Useful in the rare yet valid case that a column is both a primary and a foreign key.

Looking for pointers on what/where to add/modify test cases on this.

If this addition is welcomed I'd also go add this to the documentation.

Example

test: {
  shape: sql_table
  abc: INT {constraint: primary_key_foreign_key}
  def: INT {constraint: foreign_key}
  ghi: VARCHAR(255)
}

[alixander]

thank you @satoqz !

i think the proper way to do this is to have "constraint" keyword accept an array, as well as a string.

arrays are implemented in the parser but currently unused in the compiler for anything, so it might be a bit tricky to figure out what to do. you're welcome to try it though. you can start by adding something like this in compiler_test.go, and hacking on compiler.go until it works.

x: {
  shape: sql_table
  a: int {constraint: primary_key}
  b: int {constraint: [primary_key, foreign_key]}
}

[satoqz]

I definitely agree @alixander, especially since this would enable other niche constraint combinations such as foreign key + unique.

I'm up for the challenge and will see what I can do once there's some time available :)

[alixander]

i just implemented using arrays for class keyword, so there's an example now! https://github.com/terrastruct/d2/pull/1256/files

[satoqz]

@alixander I've updated the compiler and related parts of the code and everything seems to work as wanted, see example at the bottom of this comment.

A couple things of notice:

In below code, the else if statement would never execute to begin with since f.Primary() != nil is already a requirement to reach the switch statement.

d2/d2compiler/compile.go

Lines 479 to 490 in 6d2613e

	case "class":
	if f.Primary() != nil {
	attrs.Classes = append(attrs.Classes, scalar.ScalarString())
	} else if f.Composite != nil {
	if arr, ok := f.Composite.(*d2ir.Array); ok {
	for _, class := range arr.Values {
	if scalar, ok := class.(*d2ir.Scalar); ok {
	attrs.Classes = append(attrs.Classes, scalar.Value.ScalarString())
	}
	}
	}
	}

Therefore I've moved the handling of composite fields entirely to the top of the function, which covers class composites and constraint composites. I'm not sure how I feel about the resulting structure, it should probably be cleaned up at some point.

This also results in class arrays now being picked up by the end-to-end tests correctly.

One thing left to do is to update the test data, I've reviewed the resulting diffs and they look good to me but you'll probably want to take a look yourself as updating them via TA=1 results in ~400 changed files which I didn't want to include in this PR just yet 😄

Example:

test: {
  shape: sql_table
  a: INT
  b: INT {constraint: foreign_key}
  c: INT {constraint: [foreign_key; primary_key]}
  d: VARCHAR(255) {constraint: [foreign_key; unique]}
}

[alixander]

awesome! just a few comments and tests to see what it looks like, thanks!

[alixander]

i wonder if we should add some kind of visual separator to multiple constraints. maybe like a |? @katwangy wdyt

[satoqz]

The formula to calculate the width is obviously not perfect, but should look good for "valid" use cases up to three constraints. (i.e. assuming users don't set a field to "UNQ UNQ UNQ UNQ" or similar for some reason 😄)

[alixander]

@satoqz sorry for the looong delay here. this looks good to me. can you update this PR and run tests with TA=1 to accept all test changes?

[alixander]

oh and include a line in the changelog.md please

[satoqz]

@alixander All done :)

Preview of what it looks like with commas here: https://github.com/terrastruct/d2/blob/0ee9ce42419d586cb250ce28a23c83abe2b1cb0f/e2etests/testdata/stable/sql_table_constraints_width/dagre/sketch.exp.svg

[alixander]

looks great! ty again @satoqz