Migrations #13

nazrhom · 2017-10-20T14:58:40Z

Added support for automatic migrations between two schemas.

Supported auto-migrations are: adding or removing terms, both fields and tables, renaming will only be performed when removing a synonym or synonymous form.

resin-io-modules-versionbot · 2017-10-20T14:58:41Z

@nazrhom, status checks have failed for this PR. Please make appropriate changes and recommit.

Page- · 2017-10-23T22:18:21Z

test/sbvr/test.coffee

+				migration = AbstractSQLCompiler.postgres.diffSchemas(srcSchema, dstSchema)
+				console.log('GOT MIGRATION', migration)
+			catch e
+				throw e


You might as well drop the try/catch altogether if you're just going to rethrow :P

Page- · 2017-10-23T22:19:11Z

test/sbvr/test.coffee

+			catch e
+				throw e
+			for i in [0...Math.max(migration.length, expectation.length)]
+				expect(migration[i]).to.equal(expectation[i])


This should be able to be

expect(migration).to.deep.equal(expectation)

and avoid the need for the loop

Page- · 2017-10-23T22:19:19Z

test/sbvr/test.coffee

+				dstSchema = LF2AbstractSQLTranslator(dstLf, 'Process')
+
+				migration = AbstractSQLCompiler.postgres.diffSchemas(srcSchema, dstSchema)
+				console.log('GOT MIGRATION', migration)


Debug info?

Page- · 2017-10-23T22:21:46Z

test/sbvr/migrations.coffee

+		''', [ '''
+			ALTER TABLE "pilot-can fly-plane"
+				RENAME TO "plane-can be flown by-pilot";
+				RENAME COLUMN "pilot" TO "can be flown by-pilot";


I'm pretty sure this is invalid and should be

ALTER TABLE "plane-can be flown by-pilot" RENAME COLUMN "pilot" TO "can be flown by-pilot";

The first instruction is there to rename the the table representing the relation. I used https://www.postgresql.org/docs/9.1/static/sql-altertable.html for reference but I think that it is pretty consistently supported by different dbs.

Yes, that reference should be valid in general, my point is that the generated SQL is invalid according to that reference. Try running the create schema statements for the first model to set it up and then running the migration statements, I'm pretty sure they will fail

Oh, also the change was referring to just the line I commented on, for reference I would expect the full block to be:

[ ''' ALTER TABLE "pilot-can fly-plane" RENAME TO "plane-can be flown by-pilot"; ALTER TABLE "plane-can be flown by-pilot" RENAME COLUMN "pilot" TO "can be flown by-pilot"; ALTER TABLE "plane-can be flown by-pilot" RENAME COLUMN "can fly-plane" TO "plane"; ''' ]

or

[ ''' ALTER TABLE "pilot-can fly-plane" RENAME TO "plane-can be flown by-pilot"; ''', ''' ALTER TABLE "plane-can be flown by-pilot" RENAME COLUMN "pilot" TO "can be flown by-pilot"; ''', ''' ALTER TABLE "plane-can be flown by-pilot" RENAME COLUMN "can fly-plane" TO "plane"; ''' ]

Page- · 2017-10-23T22:23:16Z

AbstractSQLCompiler.ts

 export const postgres = generateExport(Engines.postgres, true)
 export const mysql = generateExport(Engines.mysql, true)
-export const websql = generateExport(Engines.websql, false)
+export const websql = generateExport(Engines.websql, false)


Can you put the newline back please

Page- · 2017-10-23T22:30:19Z

AbstractSQLCompiler.ts

+}
+
+const compileSchema = (abstractSqlModel: AbstractSqlModel, engine: Engines, ifNotExists: boolean): SqlModel => {
+	let { hasDependants, schemaDependencyMap } = mkSchemaDependencyMap(abstractSqlModel.tables, engine, ifNotExists) 


I'd prefer const here

Page- · 2017-10-23T22:31:54Z

AbstractSQLCompiler.ts

 	fields?: {}[]
 }

+type ReduceFn<T> = (accumulator: any, element: T) => any


I'd prefer this to be better typed, my guess is you can do that for your purposes as

type ReduceFn<T> = (accumulator: T[], element: T) => T[]

Page- · 2017-10-23T22:32:42Z

AbstractSQLCompiler.ts

+	[dependant: string]: true
+}
+
+export interface schemaDependencyMap {


Can you use CamelCase for these interfaces please, to distinguish them from vars

Page- · 2017-10-23T22:33:52Z

AbstractSQLCompiler.ts

+		}
+	}
+
+	const insFn: ReduceFn<AbstractSqlTable> = (acc, table ) => {


You have a stray space in here

Page- · 2017-10-23T22:40:36Z

AbstractSQLCompiler.ts

+//  the argument or undefined if no such element is found.
+const genDiff = <T>(insFn: ReduceFn<T>, delFn: ReduceFn<T>, modFn: ReduceFn<Pair<T>>, matchFn: MatchFn<T>, src: Array<T>, dst: Array<T>) => {
+	let split = genSplit(src, dst, matchFn)
+	return  _.flatten(_.reduce(split.ins, insFn, 


I think all this code could be made much nice with either

_.flatMap(split.mod, modFn) .concat(_.flatMap(split.del, delFn)) .concat(_.flatMap(split.ins, insFn))

and then return [] to include nothing, and return [a, b, c] to return multiple. It avoids all the need for mutation and overall should end up much nicer imo. The alternative if you only ever need to return 0 or 1 results (never > 1) is:

_.map(split.mod, modFn) .concat(_.map(split.del, delFn)) .concat(_.map(split.ins, insFn)) .reject(_.isNil)

and return for nothing, return a for something

resin-io-modules-versionbot · 2017-10-24T15:51:30Z

@nazrhom, status checks have failed for this PR. Please make appropriate changes and recommit.

resin-io-modules-versionbot · 2017-10-26T09:57:27Z

@nazrhom, status checks have failed for this PR. Please make appropriate changes and recommit.

dfunckt · 2017-10-26T08:08:49Z

AbstractSQLCompiler.ts

 	fields?: {}[]
 }

+type ReduceFn<T> = (element: T) => string | undefined


I would expect a reducing function to take two arguments, one for the current item and another for the accumulated result. Either this isn't a reducer, therefore needs a new name, or there's a bug :)

dfunckt · 2017-10-26T10:34:18Z

AbstractSQLCompiler.ts

-const compileSchema = (abstractSqlModel: AbstractSqlModel, engine: Engines, ifNotExists: boolean): SqlModel => {
-	let ifNotExistsStr: string
+const mkSchemaDependencyMap = (tables: ResourceMap<AbstractSqlTable>, engine: Engines, ifNotExists: boolean) => {
+ 	let ifNotExistsStr: string


There's a space and then a tab here.

dfunckt · 2017-10-26T10:35:01Z

AbstractSQLCompiler.ts

 		}
 	})

+	return { hasDependants: hasDependants, schemaDependencyMap: schemaDependencyMap }


The keys can be omitted.

dfunckt · 2017-10-26T10:39:47Z

AbstractSQLCompiler.ts

+			return acc
+		} 
+	}
+	, { ins: dst, del: src, mod: mod } )


I don't see a reason for using shortened names for identifiers -- what's wrong with inserted, deleted, modified, and generateSplit?

dfunckt · 2017-10-26T10:42:28Z

AbstractSQLCompiler.ts

+//  insertion, deletion or modification.
+//  All these functions are treated as reducers, their first argument
+//  will be an accumulator of all the changes up to that point and
+//  they are expected to add their changes to it and return it.


This is not a reducer then. This is a sort of a mapping function that's also allowed to have side-effects? Seeing how you define these functions in diffFields and diffSchemas later on, how you use map in this function's body and this comment, this is all needlessly confusing.

I changed them in the last commit, given the comments Page did, they used to be reducers but aren't anymore and some comments/names where lagging behind. I removed every reference to reducers to make it clear. They are just mapping functions which are not expected to have side-effects, but just map to the corresponding sql statement.

dfunckt · 2017-10-26T10:49:24Z

AbstractSQLCompiler.ts

+			return
+		}
+		if (_.isEqual(_.omit(src, ['fieldName', 'references']), _.omit(dst, ['fieldName', 'references']))) {
+			return 'RENAME COLUMN "' + src.fieldName + '" TO "' + dst.fieldName + '";\n\t'


Shouldn't these SQL statements be backend-specific (i.e. different for WebSQL, PostgreSQL, etc)? If Pine supported SQLite for example this wouldn't work, and makes me suspicious WebSQL might also have different syntax or no support even.

Good point, I assumed postgre for now, I think that most of these are pretty standard across SQL dialects but Ill check each statement.

dfunckt · 2017-10-26T10:51:17Z

AbstractSQLCompiler.ts

+					let verb = dstTable.name.split('-').slice(1, -1).join(' ')
+					return !_.isUndefined(relations[verb])
+				})
+			} else return relations


There's no need for else, you can simply return relations from outside the if. Same below.

Page- · 2017-10-27T00:57:31Z

test/sbvr/migrations.coffee

+				RENAME TO "plane-can be flown by-pilot";
+			ALTER TABLE "plane-can be flown by-pilot"
+				RENAME COLUMN "pilot" TO "can be flown by-pilot";
+				RENAME COLUMN "can fly-plane" TO "plane";\n\t


This still has the issue from #13 (comment)

Oh, also the change was referring to just the line I commented on, for reference I would expect the full block to be:

[ ''' ALTER TABLE "pilot-can fly-plane" RENAME TO "plane-can be flown by-pilot"; ALTER TABLE "plane-can be flown by-pilot" RENAME COLUMN "pilot" TO "can be flown by-pilot"; ALTER TABLE "plane-can be flown by-pilot" RENAME COLUMN "can fly-plane" TO "plane"; ''' ]

or

[ ''' ALTER TABLE "pilot-can fly-plane" RENAME TO "plane-can be flown by-pilot"; ''', ''' ALTER TABLE "plane-can be flown by-pilot" RENAME COLUMN "pilot" TO "can be flown by-pilot"; ''', ''' ALTER TABLE "plane-can be flown by-pilot" RENAME COLUMN "can fly-plane" TO "plane"; ''' ]

Ohh I kept misreading, I thought that renames on columns where in the list of actions. I will change that everywhere so that each rename has its own alter table

resin-io-modules-versionbot · 2017-10-27T14:48:36Z

@nazrhom, status checks have failed for this PR. Please make appropriate changes and recommit.

Page- · 2018-01-11T22:46:25Z

test/sbvr/test.coffee

 			expectation(result)

+	runMigration = (describe, src, dst, expectation) ->
+		it 'Migration: \n\nVocabulary: src\n' + src + '\n\nVocabulary: dst\n' + dst, ->


I think if you use

describe 'Migration', -> it 'Vocabulary: src\n' + src + '\n\nVocabulary: dst\n' + dst, ->

it might come out in the results a little nicer

Page- · 2018-01-11T22:47:59Z

AbstractSQLRules2SQL.js

        NestedIndent: function(indent) {
            var $elf = this, _fromIdx = this.input.idx;
-            return indent + "	";
+            return indent + "\t";


This is changed here but I don't see any changes to AbstractSQLRules2SQL.ometa?

Not sure about this, my AbstractSQLRules2SQL.ometa is the same as the one appearing on master, I think this is caused by some other commit that was merged without including the changes to AbstractSQLRules.js?

Page- · 2018-01-11T22:51:59Z

AbstractSQLCompiler.ts

+const extractMappings = (resource:string):[string, string] => {
+	const resourceParts = resource.split('-')
+	const subject = resourceParts[0]
+	const rest = resourceParts.slice(1).join('-')


You can make this nicer with

const [ subject, ...rest ] = resource.split('-') return [ subject, rest.join('-') ]

Page- · 2018-01-11T22:57:38Z

AbstractSQLCompiler.ts

+	const dstSDM = mkSchemaDependencyMap(dst.tables, engine, ifNotExists).schemaDependencyMap
+
+	const matchFn:MatchFn<AbstractSqlTable> = (tables, srcTable) => {
+		let match = _.find(tables, { name: srcTable.name })


This should be const

Page- · 2018-01-11T22:58:05Z

AbstractSQLCompiler.ts

+
+	const matchFn:MatchFn<AbstractSqlTable> = (tables, srcTable) => {
+		let match = _.find(tables, { name: srcTable.name })
+		if (_.isUndefined(match)) {


This would be nicer as

if (match != null) { return match }

Page- · 2018-01-11T23:08:27Z

AbstractSQLCompiler.ts

+
+	const matchFn: MatchFn<AbstractSqlField> = (fieldArray, field) => {
+		let match = _.find(fieldArray, { fieldName: field.fieldName })
+		if (_.isUndefined(match)) {


Can you shortcircuit with

if (match != null) { return match }

Page- · 2018-01-11T23:08:34Z

AbstractSQLCompiler.ts

+	}
+
+	const matchFn: MatchFn<AbstractSqlField> = (fieldArray, field) => {
+		let match = _.find(fieldArray, { fieldName: field.fieldName })


This should be const

Page- · 2018-01-11T23:09:36Z

AbstractSQLCompiler.ts

+	.concat(_.map(split.deleted, delFn))
+	.concat(_.map(split.inserted, insFn))
+
+	return _.reject(diff, _.isUndefined)


I'd prefer _.isNil to catch both null and undefined

Page- · 2018-01-11T23:10:48Z

AbstractSQLCompiler.ts

 }

+const generateSplit = <T>(src:Array<T>, dst: Array<T>, matchFn: MatchFn<T>): Split<T> => {
+	let modified: Array<Pair<T>> = []


This can be const

Page- · 2018-01-11T23:11:13Z

AbstractSQLCompiler.ts

+	let modified: Array<Pair<T>> = []
+	return _.reduce(src, (acc, value) => {
+		const match = matchFn(acc.inserted, value)
+		if (_.isUndefined(match)) {


this can be match == null

…ation from a pair of AbstractSQLModels. Change-type: minor

nazrhom requested review from CameronDiver and Page- October 20, 2017 14:58

Page- requested a review from dfunckt October 23, 2017 22:17

Page- requested changes Oct 23, 2017

View reviewed changes

nazrhom force-pushed the migrations branch from 688fec2 to 6cfed75 Compare October 24, 2017 15:51

nazrhom requested a review from Page- October 24, 2017 16:01

nazrhom force-pushed the migrations branch from 6cfed75 to b193434 Compare October 26, 2017 09:55

nazrhom requested review from Page- and removed request for Page- October 26, 2017 09:57

dfunckt reviewed Oct 26, 2017

View reviewed changes

Page- requested changes Oct 27, 2017

View reviewed changes

nazrhom force-pushed the migrations branch from b193434 to 0a59ab7 Compare October 27, 2017 14:46

nazrhom requested a review from Page- October 30, 2017 17:28

nazrhom force-pushed the migrations branch from 0a59ab7 to 141982d Compare December 13, 2017 15:22

Page- requested changes Jan 11, 2018

View reviewed changes

nazrhom force-pushed the migrations branch from b8cee47 to 3431d05 Compare April 11, 2018 09:18

nazrhom force-pushed the migrations branch from 3431d05 to fc786f1 Compare May 4, 2018 13:50

nazrhom requested a review from Page- May 4, 2018 13:55

Giovanni Garufi added 3 commits July 3, 2018 18:51

Added a diffSchema function that attempts to produce a valid sql migr…

b0345cf

…ation from a pair of AbstractSQLModels. Change-type: minor

Updated according to review

8b199d8

Rebased master branch

f3ec35c

nazrhom force-pushed the migrations branch from fc786f1 to f3ec35c Compare July 3, 2018 17:14

Migrations #13

Are you sure you want to change the base?

Migrations #13

Uh oh!

Conversation

nazrhom commented Oct 20, 2017

Uh oh!

resin-io-modules-versionbot bot commented Oct 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

resin-io-modules-versionbot bot commented Oct 24, 2017

Uh oh!

resin-io-modules-versionbot bot commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nazrhom Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

resin-io-modules-versionbot bot commented Oct 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nazrhom May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

nazrhom Oct 27, 2017 •

edited

Loading

nazrhom May 4, 2018 •

edited

Loading