mssqldef: support declare in trigger

[ytakaya]

I made a change to allow DECLARE ... to be used in the trigger syntax.

We can declare variables and cursors within the declare syntax.
Cursor can be used when we want to loop through the result set one record at a time.

This change only supports declarations, so it is just a test to check the parsing, but I can change it to meaningful tests with cursors by supporting Fetch cursor ... and OPEN cursor ... after this.

[ytakaya]

As I was making other changes, I realized that it might be better to leave the formatting of the trigger's body to the generator instead of the sqlparser#Format().

I'll make some modifications.

[k0kubun]

Out of curiosity, how many features do you plan to add to sqldef after this? Now that you've made significant contributions to sqldef, if you have many more to do, I'm thinking it might be easier for you to sometimes directly push changes to master as a committer instead of making a pull request every time. What do you think?

[ytakaya]

Thank you for your concern.

My unfinished work on sqldef is to support the following syntax for use in triggers.

• OPEN cursor, FETCH cursor ..., CLOSE cursor
• SELECT ...
• WHILE ...
• IF ... ELSE ...

The number may fluctuate, but I'm assuming that it will not be too many.
Furthermore, adding the trigger syntax requires adding the statement, which I feel is a bit different from the changes I've made in the past.
So, if possible, I would like a review to see if there are any inappropriate changes.

[ytakaya]

I modified it so that the generator can control the generation of trigger statements.

Currently, it only supports sql server, so it is a redundant process of just calling sqlparser#String().
But by preparing ast for trigger statements and putting the formatting process in the generator, it is easier to absorb the differences in syntax when supporting other DBs.

I would like to ask for advice on which policy is more appropriate, the previous one or the current one.

[k0kubun]

Furthermore, adding the trigger syntax requires adding the statement, which I feel is a bit different from the changes I've made in the past.
So, if possible, I would like a review to see if there are any inappropriate changes.

OK. But then could you give a short description for each of the previous one and the current one and summarize the pros and cons of each option first, instead of leaving a part of that information in individual comments?

It should be actually easier for the author who actually worked on a problem to see challenges in implementing a solution for the problem, so it's still your responsibility to clarify what should be considered. Otherwise, I'd need to re-implement what you did to "see if there are any inappropriate changes".

[ytakaya]

Yes, the two implementations are described below.

1. Define the statement of the trigger as string

In the first implementation, schema#Trigger is defined as follows.

type Trigger struct {
  ...
  body []string
}

This implementation keeps the string retrieved using sqlparser#String() when parsing from sqlparser#SQLNode.

• pros
  • Implementation is simple.
    • On the generator side, we can build the trigger body using only string concatenation.
    • Comparison between trigger statements is equivalent to string comparison.
• cons
  • Inconvenience occurs when there is a difference in syntax between DBs.
    • In this approach, the trigger body depends on the result of sqlparser#String(), so we need to use sqlparser to absorb the syntax difference between DBs (It's going to be adjusted in sqlparser#Format()).

2. Implement ast in each trigger statement.

In the second implementation, schema#Trigger is defined as follows.

type TriggerStatement interface {
  tStatement()
}

type Trigger struct {
  ...
  body []TriggerStatement
}

type Insert struct {
  statement string
}
func (insert Insert) tStatement()   {}

In this implementation, each trigger statement is represented by an schema#ast.
Since there is no difference between DB's in insert, it just holds a string. But if there is a difference between DB's, it adds an element to ast and assembles a statement in generator.
For example, in the if statement, there is a difference between sql server and mysql whether to useTHEN or not.
https://docs.microsoft.com/en-us/sql/t-sql/language-elements/if-else-transact-sql?view=sql-server-ver15
https://dev.mysql.com/doc/refman/5.7/en/if.html

• pros
  • The difference between DBs can be absorbed by the generator, which seems to be in line with the design of sqldef.
• cons
  • Most statements only need to hold a string, so there is a lot of redundant processing (There are not that many statements that have differences between DBs).
  • Type assertions will be needed for statement-to-statement comparisons.

func areSameTriggerStatement(stmtA, stmtB TriggerStatement) bool {
  switch stmtA := stmtA.(type) {
  case Insert:
    stmtB, ok := stmtB.(Insert)
    if !ok {
      return false
    }
    return stmtA == stmtB
  }
  return true
}

---

As I write this explanation, I am beginning to feel that the second approach is somewhat of an overkill.
For example, if the only difference is whether or not to use THEN in an if statement, I think it can be controlled by the sqlparser#Format().
What I am worried about is whether this approach is contrary to the design of sqldef.

By the way, here are the differences between the DBs that I'm finding at this stage in the statement I'm trying to add.

• FETCH cursor ...
  • In sql server, options like PRIOR and FIRST can be used.
• WHILE ...
  • whether to use DO and END WHILE
• IF ... ELSE ...
  • whether to use THEN

[k0kubun]

Thank you for writing this down. This is really helpful.

You said that everything you will add to sqldef from this point is going to be about TRIGGER (please let me know if anything not related to TRIGGER is also missing for your use case). If that's the case, especially because you said it's not too many, could you file all pull requests you'd like to merge to sqldef first? That way, I can confirm your design decision is applicable to those and compatible with them while reviewing it.

Note that this is not a request to create a single big pull request. I'd like you to file pull requests as if they were merged one by one. Once you have all changes in your repository, please use this branch as a base of your second pull request, use it as a base for your third, and so on. As you're requesting to get reviews for all of these, I'd like to see a similar kind of summary for those pull requests as well. Thank you in advance. All of your contributions have been much appreciated!

[ytakaya]

Thanks for the confirmation!
Yes, I agree to create a pull request for all triggers first.

I would like to confirm the following.

Once you have all changes in your repository, please use this branch as a base of your second pull request, use it as a base for your third, and so on.

I'm thinking of reverting to keeping the trigger statement as a string (757c233) and then deriving a branch, am I right?

[k0kubun]

I'm thinking of reverting to keeping the trigger statement as a string (757c233) and then deriving a branch, am I right?

Sounds good. For each pull request, I expect you to leave a version as the last commit that you prefer according to your discussion associated with each pull request.

[k0kubun]

So I'm starting to review your PRs, but I noticed these are not chained but simply share the same diffs. To help my review process, let me re-create PRs with your commits in this repository's branch so that we can compare the diff for each PR.

[k0kubun]

re-created the PR as #144

[k0kubun]

1. Define the statement of the trigger as string
   pros
   Implementation is simple.

I'd like to go with the simple approach as long as we don't hit a use case that is not compatible with the design. Let's try to maintain robustness around https://github.com/k0kubun/sqldef/blob/313678a42f93d5b731c8dc2feee7e663993cce3d/schema/generator.go#L1342-L1343 for now.