-
-
Notifications
You must be signed in to change notification settings - Fork 263
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
DB specific length support in JSONs/Varchars/binary data #3107
Conversation
…SqlServer workaround)
…ing deprecated image type by default)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rob-bygrave I tried to put all changes in separate commits. What do you think about these changes?
(This is related to #3103 and required to implement #3102 - so it would be great, if I can get feedback here soon, Thanks)
// see if there is a precision/scale to add (or not) | ||
int len = deployLength != 0 ? deployLength : defaultLength; | ||
if (len == Integer.MAX_VALUE) { | ||
sb.append("(max)"); // TODO: this is sqlserver specific |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove the sqlserver workaround now
@@ -15,6 +15,7 @@ class ClickHouseDbArray { | |||
mapping.put("integer[]", "Array(UInt32)"); | |||
mapping.put("bigint[]", "Array(UInt64)"); | |||
mapping.put("float[]", "Array(Float32)"); | |||
mapping.put("float4[]", "Array(Float32)"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were some issues for Clickhouse. They are in commit 81443b7
// this is a platform specific definition. So do not apply further conversions | ||
return tmp[index]; | ||
} | ||
type = tmp[index]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This detects, if there is a platform specific definition, e.g. @Column(columnDefinition = "mariadb,mysql;varchar(16000);")
This definition is taken "as is" and not converted
@@ -45,24 +45,6 @@ create table migtest_fk_set_null ( | |||
one_id UInt64 | |||
) ENGINE = Log(); | |||
|
|||
create table drop_main ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge #3108 first
I don't know, I don't understand the problem.
This isn't really correct. Most databases have a JSON database type and by default that is the type that is used. So I don't know why the first statement is: So you loose me right at the start. You are going to have to restate the issue as you understand it. |
:) @rob-bygrave I've updated the initial documentation, so that it is more clear, what this PR changes. |
Sorry, still not clear what the actual problem is. Can you state what the actual problem is? |
Hello @rbygrave, currently a Yes I can work around this by a custom (Rob, I'm sure everything is clear up to here) So I need a working Why I need a working DbJson annotation: (I think I'm losing you here) I plan to restrict the JSON that is written to the DB. So if you have This happens on our production application. We do not have any size limits on the json objects. It would be great, if the misconfiguration would be noticed by a failing save (because the JSON field is too largen) instead of a failing find (and you have invalid data in the database) Why I did this also for Varchar: Because I find it useful (If that's the issue that bothers you, tell me) What are my next steps
If still is something unclear, please ask Roland |
Problem
The reason is because SQLServer has a maximum length of 4000 for varchar and nvarchar columns. Proposed FixFor SQLServer DDL generation, instead of generating Is that the correct interpretation? Edit: Ok, I think I understand it. For myself I think I understand as a This issue also happens to impact |
Rob Notes:
For myself, I understand this as an issue with DDL generation for
varchar(x)
where x exceeds a db platform maximum. We desirevarchar(x)
to actually work across multiple database platforms and it currently does not because it does not take into account the platform specific maximum limits on varchar.This issue also happens to impact @dbjson(length=x) for any db that uses varchar to store json (e.g. SqlServer) but note that this is really a general issue with cross platform DDL generation for varchar(x) [ and similarly varbinary and DbJson ].
Robs Summary of the Fix:
This is effectively a DDL Generation change only
Types have a maximum length and fallback type. When generating DDL, based on the length the fallback type is used. Best example is MySql with
varchar
,text
,mediumtext
,longtext
types that are chained via fallback type.DDL changes - Important Notes
This changes the DDL generated:
E.g. you have
@Size(length = 16000)
String myText in your code.Problem
For platforms, that have no json database, ebean uses the
clob
type for@DbJson
without length.If length is specified (
@DbJson(length=123)
), varchar is used.The following table will show the resulting datatype for
@DbJson(...) List<String> someProp
The same issue happens also for varchars and also for varbinary (e.g.
@Size(...) String someProp
).So, we take a look at the platforms that have
json
in the table above. (for platform without json support, varchar handling is similar to the table above)@Size
@Size(max=200)
@Size(max=4096)
@Size(max=32768)
Fix
When a varchar/varbinary definition is found which is not supported at all by the platform (e.g. sqlserver
nvarchar(4096)
)or is a bad choice (e.g. mysql/db2
varchar(4096)
) because you 'waste' the row space, use the clob (respectively blob) type of that platform.IMPORTANT NOTE: This might break/change things:
E.g. you have
@Size(length = 16000) String myText
in your code.clob
instead ofvarchar(16000)
although the platform will support such large varchars (as long as you do not hit the row-size limit)If you really need a varchar(16000) here you have to explicitly override that with
@Column(columnDefinition = "mariadb,mysql;varchar(16000);")
Other changes
varbinary(max)
instead the deprecatedimage
type nowDbMigration
test will at least start