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.
There was a problem hiding this comment.
That makes sense, but doesn't address the case where the AK is not a String. Is that scenario we support? The value coming in will often be String regardless of AK type.
There was a problem hiding this comment.
Separately, we don't use Java class comparison much in import/query land. More often just trying to figure out is it Stringy, Datey, or Numbery. (Is Integer assignable from Long)
There was a problem hiding this comment.
That makes sense, but doesn't address the case where the AK is not a String. Is that scenario we support? The value coming in will often be String regardless of AK type.
I think this is meant to be a comment on my use of the isAssignalbleFrom comparison, so not this part of the code, right?
The logic in AbstractForeignKey.allowImportByAlternateKey indicates that we don't allow this unless the alternate key type is a String. Given that, we could perhaps update this to check if k is a String. How hacky is that?
Pathological case if using convert: The value given is a number (the PK value), we use convert and get back a string, then we use that string for this fetching and get a number-named thing even though we started with the PK for some other thing.
There was a problem hiding this comment.
Sorry about that. If we only allow String (which is clearly the most useful case), then just checking instanceof String seems fine (with comment). Data no longer runs through remap twice though, right?
There was a problem hiding this comment.
Correct. We are no longer running through remap twice, which is why the case I described is pathological. It would have to be a value for a field that could have number names but has integer values (not Strings that look like integers) to start out with, which would be even less common (you can't have an email address that looks like a number, for example). Less common does not mean it won't happen though, so I'm going to error on the side of caution.
Code has been updated to check for String type with comment amended. Please add further comments in that context.
| @@ -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
don't show required validation error after conversion error for the same field
…latform into fb_lookupResolutionType
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.