-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix many issues with lists and special characters #6325
base: develop
Are you sure you want to change the base?
Conversation
|
||
if (null != value) | ||
ret.put(prop.getName(), value); | ||
String propName = prop.getName(); |
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.
The ColumnInfo's in the TableInfo know the names of the columns in the result. E.g.
for (var col : queryTable.getColumns())
value = col.getValue(raw)
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 like it... so it's okay to key the map with col.getName()
instead of prop.getName()
?
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.
Unless someone is getting tricky, I think the map returned from TableSelector will be keyed with ColumnInfo.getAlias() and ColumnInfo.get() will use that.
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 agree that lookups into the raw
map via the ColumnInfo should work fine. My question is really about the return map. That is currently keyed with prop.getName()
and has far fewer entries vs. one constructed based on getColumns(). I'm concerned about making major changes to the return map.
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 don't know what is expected there.
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.
Neither do I... so I left it largely as it was before. But with ColumnInfo getting the value.
BVT and BVT-EHR suites all green (above) Daily postgres is all green, except for known, existing failures: https://teamcity.labkey.org/buildConfiguration/LabkeyTrunk_DailyPostgres_2/3385122 |
Rationale
Lists with PK names containing special characters were having a rough time. They could not handle insert, update, or delete in many cases. They also showed mangled column captions when hovering over the grid headers.
Only one of the fixes is list-specific; the rest affect other table types.
https://www.labkey.org/home/Developer/issues/issues-details.view?issueId=52070
https://www.labkey.org/home/Developer/issues/issues-details.view?issueId=52069
Changes
ListQueryUpdateService.getRow()
: ask the TableInfo for aliases instead of trying guess them. (This enables update.)appendIdentifier()
inPostgreSql91Dialect.addReselect()
. (This enables insert.)CompareType.toWhereClause()
to return aSQLFragment
, allowing the use ofappendIdentifier()
. Use it. (This enables delete... and probably resolves other issues.)FieldKey.toDisplayString()
when displaying in the grid headers