New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Javers uses deprecated type TEXT for mssql for Snapshot_State and Snapshot_Changed fields #497

Closed
wcchristian opened this Issue Feb 1, 2017 · 26 comments

Comments

Projects
None yet
3 participants
@wcchristian
Contributor

wcchristian commented Feb 1, 2017

Looking at FixedSchemaFactory as well as my installation of Microsoft Sql Server, it appears that javers is using the TEXT datatype for the STATE and CHANGED_PROPERTIES fields in the jv_snapshot table. The text datatype has been deprecated in mssql since SQL Server 2005.

http://stackoverflow.com/questions/564755/sql-server-text-type-vs-varchar-data-type
https://msdn.microsoft.com/en-us/library/ms187993.aspx

(Unable to create a unit test to show this.)

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 1, 2017

What is the real problem? Text type still exists in SQL Server 2016.
The problem with varchar(max) is that I have no idea how to set max.
STATE column holds serialized users' objects which could be arbitrary large.

@wcchristian

This comment has been minimized.

Contributor

wcchristian commented Feb 2, 2017

Looking at polyjdbc it doesn't look like it is possible to add varchar(MAX) as a column type. My worry is that when TEXT is no longer included in new versions of Sql Server things could break. I suppose this is more of a polyjdbc issue.

@wcchristian

This comment has been minimized.

Contributor

wcchristian commented Feb 2, 2017

I wrote up the following issue on polyjdbc's issue page. polyjdbc/polyjdbc#26

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 2, 2017

Fine, but how would you choose max length for state column?

@wcchristian

This comment has been minimized.

Contributor

wcchristian commented Feb 6, 2017

Unsure, it depends on polyjdbc's fix. If they add a way to set varchar max, then I would just use that. I suppose that this could be moved to a blocked status waiting on what we hear back from polyjdbc. I just figure that eventually TEXT won't be an option in sql server.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Feb 6, 2017

I'm affraid that recently, the only active commiter to polyjdbc is me :)
But even if you would have varchar2(max) in polyjdbc,
how would you choose max length for state column in javers schema?

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Mar 5, 2017

If there is no better alternative to TEXT in MS SQL Server, we will stick to TEXT. It has to be unbounded type (without max len)

@wcchristian

This comment has been minimized.

Contributor

wcchristian commented Mar 9, 2017

VARCHAR(MAX) would just be another type that polyjdbc would allow. VARCHAR(MAX) is actually bounded at a max of 2147483647 non unicode characters just like TEXT is. Ideally you would just select the new varcharMax() (If one was created.) type from polyjdbc. In MSSQL you can specify VARCHAR(MAX) just as you would specify any other data type, it's a datatype itself.

http://sqlhints.com/2016/05/11/differences-between-sql-server-text-and-varcharmax-data-type/
http://stackoverflow.com/questions/25300821/difference-between-varchar-and-text-in-mysql

@bartoszwalacik bartoszwalacik reopened this Mar 9, 2017

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Mar 9, 2017

ok, feel free to crete a pull request to https://github.com/polyjdbc/polyjdbc

@wcchristian

This comment has been minimized.

Contributor

wcchristian commented Mar 10, 2017

Sounds good. I'll take a look.

@wcchristian

This comment has been minimized.

Contributor

wcchristian commented Jun 16, 2017

I submitted the following PR to polyjdbc polyjdbc/polyjdbc#33, I will refactor what is needed (if anything) from javers and submit a PR once this is merged.

@wcchristian

This comment has been minimized.

Contributor

wcchristian commented Jun 17, 2017

@bartoszwalacik I have made the change but I am unaware of how to get the integration tests working for mssql, I created my database polly on my localhost:1433 instance, I then created user polly with pass polly with all permissions but when running the tests It fails all of them, due to what looks like it not connecting.

java.lang.reflect.UndeclaredThrowableException
	at java.lang.ThreadLocal$SuppliedThreadLocal.initialValue(ThreadLocal.java:284)
	at java.lang.ThreadLocal.setInitialValue(ThreadLocal.java:180)
	at java.lang.ThreadLocal.get(ThreadLocal.java:170)
	at org.javers.repository.sql.JaversSqlRepositoryE2ETest.getConnection(JaversSqlRepositoryE2ETest.groovy:30)
	at org.javers.repository.sql.JaversSqlRepositoryE2ETest.prepareJaversRepository_closure2(JaversSqlRepositoryE2ETest.groovy:56)
	at org.javers.repository.sql.JaversSqlRepositoryE2ETest.prepareJaversRepository_closure2(JaversSqlRepositoryE2ETest.groovy)
	at groovy.lang.Closure.call(Closure.java:423)
	at org.javers.repository.sql.SqlRepositoryBuilder$1.getConnection(SqlRepositoryBuilder.java:77)
	at org.polyjdbc.core.transaction.ExternalTransactionManager.openTransaction(ExternalTransactionManager.java:18)
	at org.polyjdbc.core.schema.SchemaManagerFactory.createInspector(SchemaManagerFactory.java:40)
	at org.polyjdbc.core.DefaultPolyJDBC.schemaInspector(DefaultPolyJDBC.java:78)
...

Not sure if there is some configuration that I am missing in setting this up. Otherwise the other unit tests are passing fine.

Note that before I create a PR I am waiting on @adamdubiel to tag a new version of polyjdbc.

EDIT: I saw that the username, pass, and db was set for a connection in MsSqlIntegrationTest.groovy that's where I got the username, pass, and dbname from

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jun 19, 2017

@wcchristian there is no automated test on MS SQL because it is a paid database running only on Windows. We don't have Windows here, so I cant help you with that :(

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jun 19, 2017

try to run manually MsSqlIntegrationTest.groovy

@wcchristian

This comment has been minimized.

Contributor

wcchristian commented Jun 19, 2017

Ahh sounds good! I will give that a shot! I will also run some manual tests and create a PR. Thanks!

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jun 19, 2017

well, it would be nice to have the new version of polyjdbc with your PR ... without that it could be hard to test how it works with JaVers. We need to ask @adamdubiel for the release

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jun 19, 2017

I will tak with him

@wcchristian

This comment has been minimized.

Contributor

wcchristian commented Jun 19, 2017

Awesome thanks, that's the plan. I asked @adamdubiel for a new release on my issue written up on polyjdbc so hopefully we can get it soon and I can update!

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jun 19, 2017

Already talked with him, the new release is coming :)

@wcchristian

This comment has been minimized.

Contributor

wcchristian commented Jun 19, 2017

Awesome! I'll take a look tonight! Thanks!

@adamdubiel

This comment has been minimized.

Contributor

adamdubiel commented Jun 19, 2017

Released as polyjdbc 0.7.1 a minute ago :)

@wcchristian

This comment has been minimized.

Contributor

wcchristian commented Jun 19, 2017

@adamdubiel Thank you! :)

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jun 19, 2017

ok, so polyjdbc 0.7.1 is released to Central, @wcchristian let me know how if MsSqlIntegrationTest.groovy is passing on clean database when Javers uses polyjdbc 0.7.1

@wcchristian

This comment has been minimized.

Contributor

wcchristian commented Jun 20, 2017

@bartoszwalacik PR is up here (#550) I noticed the Travis-CI build failed but it is also failing for the same reason on all other PRs that are currently up.

Otherwise yes, MsSqlIntegrationTest.groovy is running clean!

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jun 20, 2017

thanks @wcchristian , I will manage travis issue

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Jun 28, 2017

fixed & released in 3.3.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment