Repository navigation
Feature/10948 - #16
Merged
Merged
Conversation
What Consistency checks in PrismValueDeltaSetTripleImpl were removed. Originally, we ensured that: - All values are parent-less. This makes sense when prism value delta set triples are used in mappings, but not when used e.g. in reports (like "give me all assignments of a user"). And now we want to use them in reports, because we are replacing the special treatment of scripts in reports with more general expression evaluation. - No values are empty. This could make sense, but may be too much. In particular, PCVs can legally be empty. This check was (by mistake) non-functional anyway, because it was applied only if getParent() != null and value is a PrismObject. Why We need this to migrate script evaluation in reports to main code path for expression evaluation, which is, in turn, necessary to ensure consistent application of expression profiles. Task: 10948
What Adds TrustDescriptor to AbstractPlainStructured and SearchFilterType. Why The goal is to give the client (midPoint) the ability to attach the information about trustworthiness of expressions, scripts, and bulk actions to these objects - and preserve this information throughout the process of objects merging, cloning, [value] moving, etc. All of that is necessary to reliably implement expression profiles. Task: 10948
What Some filters didn't have trust descriptor correctly set on parsing. This is now fixed. Why This is to ensure reliable determination of expression profiles in midPoint. Task: 10948
What These two types are now JaxbVisitable. Why It is there to propagate TrustDescriptor setting to them in midPoint (Containerable because of cases when there is a Containerable embedded in JAXB bean. QueryType because there is usually a filter in a query.) Task: 10948
What For SearchFilterType we temporarily ignore immutability when setTrustDescriptor is called. Why This is to be able to set trust descriptors on primary deltas in midPoint. Temporary measure: Ultimately we'll try to resolved it in a better way. Task: 10948
This is because generated code in midPoint still refers to the old method. Task: 10948
What Added TrustDescriptor to XNode/XNodeImpl (to avoid increasing heap consumption, remove unused "parent" field). The stored trust descriptor is used when parsing PPV raw values into expressions. Why This is to provide trust descriptor for midPoint resource configuration properties (typically parsed in raw state). Task: 10948
What We PrismPropertyValue is parsed from rawExpression, the trust descriptor is propagated to the newly parsed value. Why This is to cover cases in midPoint where resource configuration property contains an ExpressionType, as is the case for "asynchronous update" resources. Task: 10948
What Added missing "accept" method to OwnedByFilterImpl and ReferencedByFilterImpl. Why They didn't have this method implemented, so expressions in them were not marked with trust descriptors in midPoint. This made TestCsvReportAllAssignments (in report-impl) fail. Task: 10948
There was a problem hiding this comment.
🟡 Changes recommended
Trust metadata can be lost during XNode copying and omitted from nested filters, while frozen objects remain mutable through new setters.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Introduces trust metadata for expression-bearing values and filters, propagating it through parsing, cloning, and visitor APIs.
Changes:
- Adds
TrustDescriptorAPIs and storage. - Propagates descriptors through property and query parsing.
- Expands visitor support and updates expression tests/utilities.
File summaries
| File | Description |
|---|---|
TestDelta.java |
Adapts expression delta tests. |
XNodeImpl.java |
Stores trust descriptors on raw nodes. |
PrismForJAXBUtil.java |
Delegates JAXB traversal. |
PrismUtilInternal.java |
Parses descriptor-aware expressions. |
ReferencedByFilterImpl.java |
Traverses nested filters. |
OwnedByFilterImpl.java |
Traverses nested filters. |
PrismReferenceValueImpl.java |
Notes missing filter traversal. |
PrismPropertyValueImpl.java |
Propagates raw-node trust. |
QueryConverterImpl.java |
Propagates trust into filter expressions. |
PrismValueDeltaSetTripleImpl.java |
Removes consistency checks. |
AbstractValueWrapper.java |
Uses centralized JAXB traversal. |
AbstractPlainStructured.java |
Stores and clones trust metadata. |
PrismAsserts2.java |
Compares expression wrappers. |
SearchFilterType.java |
Stores filter trust metadata. |
QueryType.java |
Adds JAXB visitation. |
XNode.java |
Removes parent access. |
TrustDescriptorAware.java |
Defines descriptor accessors. |
TrustDescriptor.java |
Defines trust descriptor contract. |
ValueFilter.java |
Adopts ExpressionAware. |
PrismQueryExpressionFactory.java |
Cleans nested interface declaration. |
InOidFilter.java |
Adopts ExpressionAware. |
FullTextFilter.java |
Adopts ExpressionAware. |
ExpressionAware.java |
Defines expression-bearing filter API. |
PrismPropertyValue.java |
Removes redundant supertypes. |
JaxbVisitable.java |
Centralizes recursive visitation. |
ExpressionWrapper.java |
Requires descriptor-aware payloads. |
Containerable.java |
Adds default JAXB visitation. |
Review details
Suppressed comments (2)
infra/prism-impl/src/main/java/com/evolveum/midpoint/prism/impl/xnode/XNodeImpl.java:109
- This setter bypasses
checkMutable(), unlike the surrounding XNode setters. As a result, a frozen XNode can still have its security-relevant trust provenance changed, violating theFreezablecontract. Reject mutation after freezing here as well.
public void setTrustDescriptor(TrustDescriptor trustDescriptor) {
// TODO check mutable
this.trustDescriptor = trustDescriptor;
infra/prism-api/src/main/java/com/evolveum/prism/xml/ns/_public/query_3/SearchFilterType.java:126
SearchFilterTypeis freezable and its existing content setters callcheckMutable(), but this setter allows the trust classification to change afterfreeze(). That breaks the immutable-object contract and permits a frozen filter's execution trust to be altered. Please enforce the same mutability check here.
public void setTrustDescriptor(TrustDescriptor trustDescriptor) {
//checkMutable();
this.trustDescriptor = trustDescriptor;
- Files reviewed: 27/27 changed files
- Comments generated: 7
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Why It is useful also outside QueryConverterImpl class. Task: 10948
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implementing TrustDescriptor as a basic mechanism for expression profiles in midPoint.