RFC: Use provided type to create the dstTable rather than inferring from the SQL types #1872

Merged
lunny merged 18 commits from zeripath/xorm:dump-infer-from-type into master 7 months ago

When using dumptables to convert between dialects if a struct is provided we should use it to generate the SQL types rather than infer them by mapping from the sql types.

Signed-off-by: Andrew Thornton art27@cantab.net

When using dumptables to convert between dialects if a struct is provided we should use it to generate the SQL types rather than infer them by mapping from the sql types. Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added 1 commit 9 months ago
6d1aba4e23
Use provided type to create the dstTable rather than inferring from the SQL types
Poster

In Gitea we read of reports of bool types being misconverted when converting between different dialect types. This is because the original DumpTables code only inferred the new destination type from the source table's sql type - not the actual field type.

This PR suggests if there is a Type associated with the Table we generate the destination column types from that table Type.

In Gitea we read of reports of bool types being misconverted when converting between different dialect types. This is because the original DumpTables code only inferred the new destination type from the source table's sql type - not the actual field type. This PR suggests if there is a Type associated with the Table we generate the destination column types from that table Type.
Owner
make test-sqlite#TestDumpTables

failed

=== CONT  TestDumpTables
    engine_test.go:159:
        	Error Trace:	engine_test.go:159
        	Error:      	Received unexpected error:
        	            	dest should be a struct's pointer
        	Test:       	TestDumpTables
``` make test-sqlite#TestDumpTables ``` failed ``` === CONT TestDumpTables engine_test.go:159: Error Trace: engine_test.go:159 Error: Received unexpected error: dest should be a struct's pointer Test: TestDumpTables ```
zeripath added 1 commit 8 months ago
d1be010b1d Merge branch 'master' into dump-infer-from-type
zeripath added 2 commits 8 months ago
3ff3839b59
fix test bug
Poster

I've fixed the reflection bug - sorry about leaving this one to stew for a while.

I've fixed the reflection bug - sorry about leaving this one to stew for a while.
zeripath added 1 commit 8 months ago
lunny added 1 commit 8 months ago
2e53ce7454 Merge branch 'master' into dump-infer-from-type
lunny added 1 commit 8 months ago
lunny added 1 commit 8 months ago
Poster

Looks like this change requires that we set parseTime=true on the connection string for mysql.

Is that acceptable @lunny ?

I suspect that there will be other bugs that are hidden and underlying that also require this - but I'm not certain.

OK I've managed to change to use a different thing.

~~Looks like this change requires that we set parseTime=true on the connection string for mysql.~~ ~~Is that acceptable @lunny ?~~ ~~I suspect that there will be other bugs that are hidden and underlying that also require this - but I'm not certain.~~ OK I've managed to change to use a different thing.
zeripath added 2 commits 8 months ago
fed511cc66
set parseTime=true
Owner

MsSQL test failed.

    engine_test.go:168: 
82s
494	        	Error Trace:	engine_test.go:168
82s
495	        	Error:      	Received unexpected error:
82s
496	        	            	mssql: Invalid column name 'true'.
82s
497	        	Test:       	TestDumpTables
MsSQL test failed. ``` engine_test.go:168: 82s 494 Error Trace: engine_test.go:168 82s 495 Error: Received unexpected error: 82s 496 mssql: Invalid column name 'true'. 82s 497 Test: TestDumpTables ```
Poster

grargh! this works locally!! - Oh it's MSSQL not MySQL. Let's try again!

grargh! this works locally!! - Oh it's MSSQL not MySQL. Let's try again!
zeripath added 1 commit 8 months ago
de0cf1c553
Handle bools in MSSQL
zeripath added 1 commit 8 months ago
4322c59c4f
better
zeripath added 1 commit 8 months ago
3a1b5ce682
tidb also needs parseTime too
zeripath added 1 commit 8 months ago
efebf2525f
remove fprintf
Poster

OK so I think I need to move code from session.Get() into convert.go to make this possible. - Done

OK so I think I need to move code from `session.Get()` into convert.go to make this possible. - Done
zeripath added 1 commit 8 months ago
d64cf2f2d2
use session for import
zeripath added 1 commit 7 months ago
zeripath added 1 commit 7 months ago
ed8e7e426a
ugh
zeripath added 1 commit 7 months ago
26cbce14e0
Fix mssql timezone
Poster

@lunny looks like this may finally be passing tests!

I just need to check that the dumps it is producing actually are correct.

@lunny looks like this may finally be passing tests! I just need to check that the dumps it is producing actually are correct.
Poster

I think I've got it and that the times are now correct.

I guess it would be useful to try building a gitea with this to ensure that the dump is now completely correct there.

I'm not completely au fait with the magic of go.mod aliasing so if you're able to do that easily that would be best but I can try to figure it out if you want me to have a go.

I think I've got it and that the times are now correct. I guess it would be useful to try building a gitea with this to ensure that the dump is now completely correct there. I'm not completely au fait with the magic of go.mod aliasing so if you're able to do that easily that would be best but I can try to figure it out if you want me to have a go.
Owner

Since all tests PASS, let's merge it and test it with Gitea.

Since all tests PASS, let's merge it and test it with Gitea.
lunny merged commit 912c2524f8 into master 7 months ago
lunny added the
kind/bug
label 7 months ago
zeripath deleted branch dump-infer-from-type 7 months ago
Poster

This isn't correct - as the dump doesn't work on Gitea. Sorry - you beat me to it.

This isn't correct - as the dump doesn't work on Gitea. Sorry - you beat me to it.
Poster

OK so the issue is convert.Conversion types are failing to be handled correctly.

OK so the issue is convert.Conversion types are failing to be handled correctly.
lunny referenced this issue from a commit 7 months ago
lunny referenced this issue from a commit 7 months ago
continuous-integration/drone/pr Build is passing
The pull request has been merged as 912c2524f8.
Sign in to join this conversation.
Loading…
There is no content yet.