Repository navigation
KEYCLOAK-12473 Add possibility to specify length of event detail when… - #6576
Conversation
|
Would it not make more sense to just increase the size of the field in the DB? |
|
@stianst the size of the filed is 2550 atm, how would you determine what size should be enough and how would you ensure the redirect_uri param has lesser size? I think increasing the size would be more like workaround than a proper solution. |
d45c19d to
918cb28
Compare
Yes, it would be possible to use CLOB or similar for empty database. For big databases with millions of records, this could involve (depending on DB vendor) downtime. The proposed way is conservative in this regard. |
There was a problem hiding this comment.
| if (maxDetailLength > 0) { | |
| if (maxDetailLength > 0 && event.getDetails() != null) { |
|
If we're going with trimming, which I would argue is a workaround as you are loosing data, we should not require users to configure it (there's a max size of the field in the db and we should just automatically trim it to fit, without user having to first get and issue then figure out what length to set it to), we also should make it obvious that the field has been trimmed (perhaps adding ... to the end) |
423773a to
4901b6b
Compare
|
@stianst I've added ... to the trimmed string to make it obvious it was trimmed, there is also warning in log about that. We've discussed the approach you're suggesting (trim it to the maximum length left) in puma team, and we decided to go with adding possibility to configure it for use-cases where the problems appears. The reason for that was different coding has different length of characters. I can explore this approach further. |
|
@stianst it seems to me trimming the length to maximum length left is not the trivial thing to do. Different databases can have different characters encoding and each encoding uses different length for each character. |
|
So basically what we are saying is that if someone runs into an issue in production, then they need to discover this config option, then they somehow need to figure out what to set it to (which you are saying is far from trivial). Not really an ideal solution. |
|
Isn't the field a varchar? So it should support X characters regardless of encoding? |
|
I'm investigating it for all supported databases. |
It's true for all these databases: MySQL, Postgresql, MariaDB, MariaDB Galera, MSSql. Unfortunately for Oracle database is VARCHAR2 datatype used, by default oracle uses max_size in BYTEs so the column is able to hold 2550 bytes of character. For redirect_uri we're fine as there are only non US-ASCII characters or encoded ones but the problem is with username (which is also part of the login event detail). The username doesn't have any limitation so there comes the issue with finding out the actual length stored. I could come up with following solutions, but there might be better one.
|
|
So you are saying that setting max value to 2500 would work for all DB except Oracle, and for Oracle it would still work for URLs? If so I'd set it to 2500 and make it configurable in case some runs into issues with Oracle, which seems less likely if it's not an issue for URLs. |
|
No, that's not what I'm saying. I was trying to say that for all DBs except Oracle would be possible to relatively easily count the number of characters and based on that we would be able to cut the redirect_uri to make sure we cut the minimum characters. For Oracle it's not so easy to count. IMO if we set default to 1500 we should be ok for almost all cases. |
|
I think that make it configurable with default == 1500 and 0 == "do not trim" can do the trick. |
|
@stianst wdyt? Should we go with 1500 characters default cut? |
There was a problem hiding this comment.
LGTM - just some minor comments left
… storing to database
4901b6b to
edc6e73
Compare
… storing to database