Skip to content

Commit

Permalink
addressed the comment and feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@akashthawaitcc
akashthawaitcc authored and Vivek Yadav committed Dec 27, 2024
1 parent 5bfbe79 commit 4a2ca12
Show file tree
Hide file tree
Showing 6 changed files with 164 additions and 87 deletions.
2 changes: 1 addition & 1 deletion internal/reports/report_helpers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ func buildTableReportBody(conv *internal.Conv, tableId string, issues map[string
}

// added if to add table level issue
if p.severity == warning && len(tableLevelIssues) != 0 {
if p.severity == Errors && len(tableLevelIssues) != 0 {
for _, issue := range tableLevelIssues {
switch issue {
case internal.TypeMismatch:
Expand Down
79 changes: 35 additions & 44 deletions sources/common/toddl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,23 +110,34 @@ func GenerateExpressionDetailList(spschema ddl.Schema) []internal.ExpressionDeta
return expressionDetailList
}

// removeCheckConstraint this method will remove the constraint which has error
func RemoveCheckConstraints(checkConstraints []ddl.CheckConstraint, exprVerificationOutputs []internal.ExpressionVerificationOutput) []ddl.CheckConstraint {
unwantedExprIds := make(map[string]bool)
for _, ev := range exprVerificationOutputs {
func GetIssue(result internal.VerifyExpressionsOutput) map[string][]internal.SchemaIssue {
exprOutputsByTable := make(map[string][]internal.ExpressionVerificationOutput)
issues := make(map[string][]internal.SchemaIssue)
for _, ev := range result.ExpressionVerificationOutputList {
if !ev.Result {
unwantedExprIds[ev.ExpressionDetail.ExpressionId] = true
tableId := ev.ExpressionDetail.Metadata["tableId"]
exprOutputsByTable[tableId] = append(exprOutputsByTable[tableId], ev)
}
}

outIdx := 0
for _, checkConstraint := range checkConstraints {
if !unwantedExprIds[checkConstraint.ExprId] {
checkConstraints[outIdx] = checkConstraint
outIdx++
for tableId, exprOutputs := range exprOutputsByTable {

for _, ev := range exprOutputs {

for key, issue := range ErrorTypeMapping {
if strings.Contains(ev.Err.Error(), key) {
issues[tableId] = append(issues[tableId], issue)
break

}
}

}

}
return checkConstraints[:outIdx]

return issues

}

// VerifyExpression this function will use expression_api to validate check constraint expressions and add the relevant error
Expand All @@ -146,44 +157,24 @@ func (ss *SchemaToSpannerImpl) VerifyExpressions(conv *internal.Conv) error {
if result.ExpressionVerificationOutputList == nil {
return result.Err
}
issueTypes := GetIssue(result)
if len(issueTypes) > 0 {
for tableId, issues := range issueTypes {

exprOutputsByTable := make(map[string][]internal.ExpressionVerificationOutput)
for _, ev := range result.ExpressionVerificationOutputList {
if !ev.Result {
tableId := ev.ExpressionDetail.Metadata["tableId"]
exprOutputsByTable[tableId] = append(exprOutputsByTable[tableId], ev)
}
}

for tableId, exprOutputs := range exprOutputsByTable {

spschema := conv.SpSchema[tableId]
spschema.CheckConstraints = RemoveCheckConstraints(spschema.CheckConstraints, exprOutputs)
conv.SpSchema[tableId] = spschema

for _, ev := range exprOutputs {

var issueType internal.SchemaIssue

for key, issue := range ErrorTypeMapping {
if strings.Contains(ev.Err.Error(), key) {
issueType = issue
break
}
}

if _, exists := conv.SchemaIssues[tableId]; !exists {
conv.SchemaIssues[tableId] = internal.TableIssues{
TableLevelIssues: []internal.SchemaIssue{},
for _, issue := range issues {
if _, exists := conv.SchemaIssues[tableId]; !exists {
conv.SchemaIssues[tableId] = internal.TableIssues{
TableLevelIssues: []internal.SchemaIssue{},
}
}
}

tableIssue := conv.SchemaIssues[tableId]
tableIssue := conv.SchemaIssues[tableId]

if !IsSchemaIssuePresent(tableIssue.TableLevelIssues, issueType) {
tableIssue.TableLevelIssues = append(tableIssue.TableLevelIssues, issueType)
if !IsSchemaIssuePresent(tableIssue.TableLevelIssues, issue) {
tableIssue.TableLevelIssues = append(tableIssue.TableLevelIssues, issue)
}
conv.SchemaIssues[tableId] = tableIssue
}
conv.SchemaIssues[tableId] = tableIssue
}
}

Expand Down
107 changes: 107 additions & 0 deletions sources/common/toddl_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,18 @@
package common

import (
"context"
"errors"
"reflect"
"testing"

"github.com/GoogleCloudPlatform/spanner-migration-tool/common/constants"
"github.com/GoogleCloudPlatform/spanner-migration-tool/internal"
"github.com/GoogleCloudPlatform/spanner-migration-tool/mocks"
"github.com/GoogleCloudPlatform/spanner-migration-tool/schema"
"github.com/GoogleCloudPlatform/spanner-migration-tool/spanner/ddl"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
)

func Test_quoteIfNeeded(t *testing.T) {
Expand Down Expand Up @@ -584,3 +588,106 @@ func TestSpannerSchemaApplyExpressions(t *testing.T) {
})
}
}

func TestVerifyCheckConstraintExpressions(t *testing.T) {
tests := []struct {
name string
expressions []ddl.CheckConstraint
expectedResults []internal.ExpressionVerificationOutput
expectedCheckConstraint []ddl.CheckConstraint
expectedResponse bool
}{
{
name: "AllValidExpressions",
expressions: []ddl.CheckConstraint{
{Expr: "(col1 > 0)", ExprId: "expr1", Name: "check1"},
},
expectedResults: []internal.ExpressionVerificationOutput{
{Result: true, Err: nil, ExpressionDetail: internal.ExpressionDetail{Expression: "(col1 > 0)", Type: "CHECK", Metadata: map[string]string{"tableId": "t1"}, ExpressionId: "expr1"}},
},
expectedCheckConstraint: []ddl.CheckConstraint{
{Expr: "(col1 > 0)", ExprId: "expr1", Name: "check1"},
},
expectedResponse: false,
},
{
name: "InvalidSyntaxError",
expressions: []ddl.CheckConstraint{
{Expr: "(col1 > 0)", ExprId: "expr1", Name: "check1"},
{Expr: "(col1 > 18", ExprId: "expr2", Name: "check2"},
},
expectedResults: []internal.ExpressionVerificationOutput{
{Result: true, Err: nil, ExpressionDetail: internal.ExpressionDetail{Expression: "(col1 > 0)", Type: "CHECK", Metadata: map[string]string{"tableId": "t1"}, ExpressionId: "expr1"}},
{Result: false, Err: errors.New("Syntax error ..."), ExpressionDetail: internal.ExpressionDetail{Expression: "(col1 > 18", Type: "CHECK", Metadata: map[string]string{"tableId": "t1"}, ExpressionId: "expr2"}},
},
expectedCheckConstraint: []ddl.CheckConstraint{
{Expr: "(col1 > 0)", ExprId: "expr1", Name: "check1"},
{Expr: "(col1 > 18", ExprId: "expr2", Name: "check2"},
},
expectedResponse: true,
},
{
name: "NameError",
expressions: []ddl.CheckConstraint{
{Expr: "(col1 > 0)", ExprId: "expr1", Name: "check1"},
{Expr: "(col1 > 18)", ExprId: "expr2", Name: "check2"},
},
expectedResults: []internal.ExpressionVerificationOutput{
{Result: true, Err: nil, ExpressionDetail: internal.ExpressionDetail{Expression: "(col1 > 0)", Type: "CHECK", Metadata: map[string]string{"tableId": "t1"}, ExpressionId: "expr1"}},
{Result: false, Err: errors.New("Unrecognized name ..."), ExpressionDetail: internal.ExpressionDetail{Expression: "(col1 > 18)", Type: "CHECK", Metadata: map[string]string{"tableId": "t1"}, ExpressionId: "expr2"}},
},
expectedCheckConstraint: []ddl.CheckConstraint{
{Expr: "(col1 > 0)", ExprId: "expr1", Name: "check1"},
{Expr: "(col1 > 18)", ExprId: "expr2", Name: "check2"},
},
expectedResponse: true,
},
{
name: "TypeError",
expressions: []ddl.CheckConstraint{
{Expr: "(col1 > 0)", ExprId: "expr1", Name: "check1"},
{Expr: "(col1 > 18)", ExprId: "expr2", Name: "check2"},
},
expectedResults: []internal.ExpressionVerificationOutput{
{Result: true, Err: nil, ExpressionDetail: internal.ExpressionDetail{Expression: "(col1 > 0)", Type: "CHECK", Metadata: map[string]string{"tableId": "t1"}, ExpressionId: "expr1"}},
{Result: false, Err: errors.New("No matching signature for operator"), ExpressionDetail: internal.ExpressionDetail{Expression: "(col1 > 18)", Type: "CHECK", Metadata: map[string]string{"tableId": "t1"}, ExpressionId: "expr2"}},
},
expectedCheckConstraint: []ddl.CheckConstraint{
{Expr: "(col1 > 0)", ExprId: "expr1", Name: "check1"},
{Expr: "(col1 > 18)", ExprId: "expr2", Name: "check2"},
},
expectedResponse: true,
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
mockAccessor := new(mocks.MockExpressionVerificationAccessor)
handler := &SchemaToSpannerImpl{ExpressionVerificationAccessor: mockAccessor}

conv := internal.MakeConv()

ctx := context.Background()

conv.SpSchema = map[string]ddl.CreateTable{
"t1": {
Name: "table1",
Id: "t1",
PrimaryKeys: []ddl.IndexKey{{ColId: "c1"}},
ColIds: []string{"c1"},
ColDefs: map[string]ddl.ColumnDef{
"c1": {Name: "col1", Id: "c1", T: ddl.Type{Name: ddl.Int64}},
},
CheckConstraints: tc.expressions,
},
}

mockAccessor.On("VerifyExpressions", ctx, mock.Anything).Return(internal.VerifyExpressionsOutput{
ExpressionVerificationOutputList: tc.expectedResults,
})
handler.VerifyExpressions(conv)
assert.Equal(t, conv.SpSchema["t1"].CheckConstraints, tc.expectedCheckConstraint)

})
}
}
14 changes: 7 additions & 7 deletions sources/mysql/infoschema_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func TestProcessSchemaMYSQL(t *testing.T) {
},
},
{
query: `SELECT COUNT\(\*\) FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA = 'INFORMATION_SCHEMA' AND TABLE_NAME = 'CHECK_CONSTRAINTS';`,
query: regexp.QuoteMeta(`SELECT COUNT(*) FROM INFORMATION_SCHEMA.TABLES WHERE (TABLE_SCHEMA = 'information_schema' OR TABLE_SCHEMA = 'INFORMATION_SCHEMA') AND TABLE_NAME = 'CHECK_CONSTRAINTS';`),
args: nil,
cols: []string{"count"},
rows: [][]driver.Value{
Expand Down Expand Up @@ -96,7 +96,7 @@ func TestProcessSchemaMYSQL(t *testing.T) {
cols: []string{"INDEX_NAME", "COLUMN_NAME", "SEQ_IN_INDEX", "COLLATION", "NON_UNIQUE"},
},
{
query: `SELECT COUNT\(\*\) FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA = 'INFORMATION_SCHEMA' AND TABLE_NAME = 'CHECK_CONSTRAINTS';`,
query: regexp.QuoteMeta(`SELECT COUNT(*) FROM INFORMATION_SCHEMA.TABLES WHERE (TABLE_SCHEMA = 'information_schema' OR TABLE_SCHEMA = 'INFORMATION_SCHEMA') AND TABLE_NAME = 'CHECK_CONSTRAINTS';`),
args: nil,
cols: []string{"count"},
rows: [][]driver.Value{
Expand Down Expand Up @@ -145,7 +145,7 @@ func TestProcessSchemaMYSQL(t *testing.T) {
},
},
{
query: `SELECT COUNT\(\*\) FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA = 'INFORMATION_SCHEMA' AND TABLE_NAME = 'CHECK_CONSTRAINTS';`,
query: regexp.QuoteMeta(`SELECT COUNT(*) FROM INFORMATION_SCHEMA.TABLES WHERE (TABLE_SCHEMA = 'information_schema' OR TABLE_SCHEMA = 'INFORMATION_SCHEMA') AND TABLE_NAME = 'CHECK_CONSTRAINTS';`),
args: nil,
cols: []string{"count"},
rows: [][]driver.Value{
Expand Down Expand Up @@ -181,7 +181,7 @@ func TestProcessSchemaMYSQL(t *testing.T) {
cols: []string{"INDEX_NAME", "COLUMN_NAME", "SEQ_IN_INDEX", "COLLATION", "NON_UNIQUE"},
},
{
query: `SELECT COUNT\(\*\) FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA = 'INFORMATION_SCHEMA' AND TABLE_NAME = 'CHECK_CONSTRAINTS';`,
query: regexp.QuoteMeta(`SELECT COUNT(*) FROM INFORMATION_SCHEMA.TABLES WHERE (TABLE_SCHEMA = 'information_schema' OR TABLE_SCHEMA = 'INFORMATION_SCHEMA') AND TABLE_NAME = 'CHECK_CONSTRAINTS';`),
args: nil,
cols: []string{"count"},
rows: [][]driver.Value{
Expand Down Expand Up @@ -237,7 +237,7 @@ func TestProcessSchemaMYSQL(t *testing.T) {
cols: []string{"INDEX_NAME", "COLUMN_NAME", "SEQ_IN_INDEX", "COLLATION", "NON_UNIQUE"},
},
{
query: `SELECT COUNT\(\*\) FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA = 'INFORMATION_SCHEMA' AND TABLE_NAME = 'CHECK_CONSTRAINTS';`,
query: regexp.QuoteMeta(`SELECT COUNT(*) FROM INFORMATION_SCHEMA.TABLES WHERE (TABLE_SCHEMA = 'information_schema' OR TABLE_SCHEMA = 'INFORMATION_SCHEMA') AND TABLE_NAME = 'CHECK_CONSTRAINTS';`),
args: nil,
cols: []string{"count"},
rows: [][]driver.Value{
Expand Down Expand Up @@ -418,7 +418,7 @@ func TestProcessData_MultiCol(t *testing.T) {
rows: [][]driver.Value{{"test"}},
},
{
query: `SELECT COUNT\(\*\) FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA = 'INFORMATION_SCHEMA' AND TABLE_NAME = 'CHECK_CONSTRAINTS';`,
query: regexp.QuoteMeta(`SELECT COUNT(*) FROM INFORMATION_SCHEMA.TABLES WHERE (TABLE_SCHEMA = 'information_schema' OR TABLE_SCHEMA = 'INFORMATION_SCHEMA') AND TABLE_NAME = 'CHECK_CONSTRAINTS';`),
args: nil,
cols: []string{"count"},
rows: [][]driver.Value{
Expand Down Expand Up @@ -530,7 +530,7 @@ func TestProcessSchema_Sharded(t *testing.T) {
rows: [][]driver.Value{{"test"}},
},
{
query: `SELECT COUNT\(\*\) FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA = 'INFORMATION_SCHEMA' AND TABLE_NAME = 'CHECK_CONSTRAINTS';`,
query: regexp.QuoteMeta(`SELECT COUNT(*) FROM INFORMATION_SCHEMA.TABLES WHERE (TABLE_SCHEMA = 'information_schema' OR TABLE_SCHEMA = 'INFORMATION_SCHEMA') AND TABLE_NAME = 'CHECK_CONSTRAINTS';`),
args: nil,
cols: []string{"count"},
rows: [][]driver.Value{
Expand Down
3 changes: 1 addition & 2 deletions ui/src/app/services/fetch/fetch.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ import ICreateSequence from 'src/app/model/auto-gen'
providedIn: 'root',
})
export class FetchService {
// private url: string = window.location.origin
private url: string = 'http://localhost:8080'
private url: string = window.location.origin
constructor(private http: HttpClient) {}

connectTodb(payload: IDbConfig, dialect: string) {
Expand Down
46 changes: 13 additions & 33 deletions webv2/api/schema.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -561,46 +561,26 @@ func (expressionVerificationHandler *ExpressionsVerificationHandler) VerifyCheck
return
}

exprOutputsByTable := make(map[string][]internal.ExpressionVerificationOutput)
for _, ev := range result.ExpressionVerificationOutputList {
if !ev.Result {
tableId := ev.ExpressionDetail.Metadata["tableId"]
exprOutputsByTable[tableId] = append(exprOutputsByTable[tableId], ev)
}
}

for tableId, exprOutputs := range exprOutputsByTable {
issueTypes := common.GetIssue(result)
if len(issueTypes) > 0 {
hasErrorOccurred = true
spschema := sessionState.Conv.SpSchema[tableId]
spschema.CheckConstraints = common.RemoveCheckConstraints(spschema.CheckConstraints, exprOutputs)
sessionState.Conv.SpSchema[tableId] = spschema

for _, ev := range exprOutputs {

var issueType internal.SchemaIssue

for key, issue := range common.ErrorTypeMapping {
if strings.Contains(ev.Err.Error(), key) {
issueType = issue
break
for tableId, issues := range issueTypes {
for _, issue := range issues {
if _, exists := sessionState.Conv.SchemaIssues[tableId]; !exists {
sessionState.Conv.SchemaIssues[tableId] = internal.TableIssues{
TableLevelIssues: []internal.SchemaIssue{},
}
}
}

if _, exists := sessionState.Conv.SchemaIssues[tableId]; !exists {
sessionState.Conv.SchemaIssues[tableId] = internal.TableIssues{
TableLevelIssues: []internal.SchemaIssue{},
}
}
tableIssue := sessionState.Conv.SchemaIssues[tableId]

tableIssue := sessionState.Conv.SchemaIssues[tableId]
if !utilities.IsSchemaIssuePresent(tableIssue.TableLevelIssues, issue) {
tableIssue.TableLevelIssues = append(tableIssue.TableLevelIssues, issue)
}

if !utilities.IsSchemaIssuePresent(tableIssue.TableLevelIssues, issueType) {
tableIssue.TableLevelIssues = append(tableIssue.TableLevelIssues, issueType)
sessionState.Conv.SchemaIssues[tableId] = tableIssue
}

sessionState.Conv.SchemaIssues[tableId] = tableIssue
}

}

session.UpdateSessionFile()
Expand Down

0 comments on commit 4a2ca12

Please sign in to comment.