Add audit detail change for dataclass lineage events #7032
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds audit detail change functionality for dataclass lineage events by refactoring parent field population logic and improving audit comparison for experimental inputs.
- Refactored parent field population methods from SampleTypeUpdateServiceDI to ExperimentServiceImpl for reuse
- Enhanced audit handling to treat parent input values as sets rather than ordered lists
- Added permission checks and lineage field population for dataclass operations
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| SampleTypeUpdateServiceDI.java | Removed duplicate parent field logic and delegated to ExperimentServiceImpl |
| ExperimentServiceImpl.java | Added centralized parent field population methods and updated parent lookup signatures |
| ExpDataClassDataTableImpl.java | Added parent field population and permission checks for dataclass operations |
| ExperimentService.java | Removed deprecated parent lookup method signatures from API |
| AuditHandler.java | Enhanced audit comparison to treat parent inputs as unordered sets |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (isExpInput && oldValue != null && newValue != null) | ||
| { | ||
| // For parent inputs, the order of the values does not matter, so compare as sets | ||
| Set<String> oldSet = new HashSet<>(Arrays.asList(oldValue.toString().split(","))); | ||
| Set<String> newSet = new HashSet<>(Arrays.asList(newValue.toString().split(","))); | ||
| if (oldSet.equals(newSet) && !isExtraAuditField) | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The split operation on comma-separated values doesn't handle quoted values correctly. Names containing commas are quoted (as seen in line 2555-2556 of ExperimentServiceImpl.java), but this split will break quoted entries. Consider using a CSV parser or the same quoting logic used when creating the values.
| Map<String, String> importAliases = _dataClass.getImportAliases(); | ||
| for (String col : columns) | ||
| { | ||
| if (ExperimentService.isInputOutputColumn(col) || Strings.CI.equals("parent",col) || importAliases.containsKey(col)) |
There was a problem hiding this comment.
The method call Strings.CI.equals("parent",col) has the parameters in an unusual order. Typically the expected value comes first: Strings.CI.equals(col, "parent"). This current order could be confusing and inconsistent with common patterns.
| if (ExperimentService.isInputOutputColumn(col) || Strings.CI.equals("parent",col) || importAliases.containsKey(col)) | |
| if (ExperimentService.isInputOutputColumn(col) || Strings.CI.equals(col, "parent") || importAliases.containsKey(col)) |
| List<Map<String, Object>> result = new ArrayList<>(keys.size()); | ||
| for (Map<String, Object> row : keys) | ||
| { | ||
| Map<String, Object> materialMap = getDataMap(row, user, container, true,false); |
There was a problem hiding this comment.
The boolean parameters true,false are unclear without parameter names. Consider using named parameters or constants to make the intent clear, especially since this pattern appears to indicate allowCrossContainer=true, addInputs=false.
| } | ||
| else | ||
| { | ||
| if (isExpInput && oldValue != null && newValue != null) |
There was a problem hiding this comment.
This isExpInput related logic does not seem appropriate for the AuditHandler interface. Is there a way that this can be registered as a part of a current or new audit handler that is only invoked for the necessary tables?
There was a problem hiding this comment.
Good point. This has been brought up a few times but hasn't been done due to difficulty. I added a TODO comment as a reminder.
| } | ||
|
|
||
| protected Map<String, Object> _select(Container container, Long rowId, String lsid, String name, Long classId, boolean allowCrossContainer) throws SQLException | ||
| protected Map<String, Object> getDataMap(Map<String, Object> keys, User user, Container container, boolean allowCrossContainer, boolean addInputs) throws SQLException |
There was a problem hiding this comment.
nit: Annotate as @Nullable
| throw new InvalidKeyException("Value must be supplied for key field 'rowid' or 'lsid' or 'name'", keys); | ||
|
|
||
| return _select(container, rowId, lsid, name, classId, allowCrossContainer); | ||
| return getDataMap(keys, user, container, allowCrossContainer, true); |
There was a problem hiding this comment.
classId no longer needs to be computed since it is not being handed through.
|
|
||
| public void addRowsParentsFields(Set<Identifiable> seeds, Map<Integer, Map<String, Object>> dataRows, User user, Container container) | ||
| { | ||
| Map<String, Pair<Set<ExpMaterial>, Set<ExpData>>> parents = ExperimentServiceImpl.get().getParentMaterialAndDataMap(container, user, new HashSet<>(seeds)); |
There was a problem hiding this comment.
Why new up a HashSet here?
Rationale
Allowing sources to have parents in Apps is a relatively new feature, compared with sample's parent. Sample lineage change details are needed for Timeline display, and that's backed by audit event's old/new record map. Sources, however, lacked such detailed audit. A recent refactor of audit added source lineage audit for some scenarios. This PR ensures all insert/update/delete of sources' lineage have proper audit details.
Related Pull Requests
Changes