Issue 52098, 49422: when looking up by alternate keys do that first so number names will resolve well #6642
Conversation
…_lookupResolutionType # Conflicts: # api/src/org/labkey/api/dataiterator/DataIteratorContext.java # api/src/org/labkey/api/dataiterator/SimpleTranslator.java
|
|
||
| public enum LookupResolutionType | ||
| { | ||
| primaryKey(true, false, true), // known that the use will always supply the pk value |
There was a problem hiding this comment.
| primaryKey(true, false, true), // known that the use will always supply the pk value | |
| primaryKey(true, false, true), // known that the user will always supply the pk value |
…w for alternate key (for trigger scripts?)
| TableSelector ts = createSelector(pkCol, altKeyCol, k); | ||
| ts.fillMultiValuedMap(map); | ||
| vs = map.get(k); | ||
| if (altKeyCol.getJdbcType().getJavaClass().isAssignableFrom(k.getClass())) |
There was a problem hiding this comment.
This doesn't seem quite right. I expect k will almost always be a string, should we attempt a convert?
There was a problem hiding this comment.
Trying to remember why I needed this... I think it may have been when k was an integer after having already been converted and we were going through a second time, so perhaps is no longer needed. I'll take it out and retest.
There was a problem hiding this comment.
Ah, yes. This is needed because of the system fields (like users) that initially come through with rowIds. Without this if statement (or some if statement), we get a SQL error because it is trying to compare the rowId to the string alternate key (e.g., email). I'm open to suggestions for other checks here, but this seems less expensive than a try-catch around a call to convert.
| { | ||
| // Issue 48347: if the lookup field has a "Lookup Validator", then treat the missing values as an error | ||
| boolean hasValidator = pd != null && pd.getValidators().stream().anyMatch(v -> PropertyValidatorType.Lookup.getLabel().equalsIgnoreCase(v.getName())); | ||
|
|
||
| RemapMissingBehavior missing = remapMissingBehavior; | ||
| if (missing == null) | ||
| missing = col.isRequired() || hasValidator ? RemapMissingBehavior.Error : RemapMissingBehavior.Null; | ||
| c = new RemapPostConvertColumn(c, fromIndex, col, missing, true); | ||
| // For enum tables, we should not be using number names for the values, so we will look up by primary key first (resolving rowIds) then alternate. |
There was a problem hiding this comment.
I can't picture a situation where would would want to use RemapMissingBehavior.Null?
There was a problem hiding this comment.
Ah yes, we were previously doing this for the pre-trigger phase, but not anymore, so, as discussed, I agree we should us OriginalValue here instead of Null. this may change existing behavior when we are not validating lookups, but for the better.
| @@ -1262,10 +1261,11 @@ public int addConvertColumn(ColumnInfo col, int fromIndex) | |||
| * @param fromIndex Source column to create the output column from. | |||
| * @param mvIndex Missing value column index. | |||
| * @param remapMissingBehavior The behavior desired when remapping fails. If null, indicate an error if the column is required or null if not required. | |||
| * @param hasBeenRemapped Indicates if remapping of lookup columns has happened or not | |||
There was a problem hiding this comment.
I don't particularly like this name. I think it would be better to have an "active" name here. E.g. remapLookups or withLookupRemapping.
There was a problem hiding this comment.
Updated to withLookupRemapping
Rationale
Issue 52098: Sample statuses with number labels may not resolve to the correct status
Issue 49422: List lookups with number-like values may not resolve to the correct status
Related Pull Requests
Changes
LookupResolutionTypeas a replacement forallowLookupByAlternateKeySimpleTranslatorto allow for the different orderings for lookup resolutionDataIteratorContextwith a newhasBeenCoercedproperty (could perhaps be named better?) so we don't try to re-convert lookups that have already been converted.