-
Notifications
You must be signed in to change notification settings - Fork 245
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
refactor: bind policy scope to policy context hierarchy #4535
refactor: bind policy scope to policy context hierarchy #4535
Conversation
...itor-core/src/main/java/org/eclipse/edc/connector/policy/monitor/PolicyMonitorExtension.java
Fixed
Show fixed
Hide fixed
...st-core/src/main/java/org/eclipse/edc/iam/identitytrust/core/DcpScopeExtractorExtension.java
Fixed
Show fixed
Hide fixed
...st-core/src/main/java/org/eclipse/edc/iam/identitytrust/core/DcpScopeExtractorExtension.java
Fixed
Show fixed
Hide fixed
...engine-spi/src/main/java/org/eclipse/edc/policy/engine/spi/AtomicConstraintRuleFunction.java
Dismissed
Show dismissed
Hide dismissed
...engine-spi/src/main/java/org/eclipse/edc/policy/engine/spi/AtomicConstraintRuleFunction.java
Dismissed
Show dismissed
Hide dismissed
...engine-spi/src/main/java/org/eclipse/edc/policy/engine/spi/AtomicConstraintRuleFunction.java
Dismissed
Show dismissed
Hide dismissed
...engine-spi/src/main/java/org/eclipse/edc/policy/engine/spi/AtomicConstraintRuleFunction.java
Dismissed
Show dismissed
Hide dismissed
spi/common/policy-engine-spi/src/main/java/org/eclipse/edc/policy/engine/spi/PolicyEngine.java
Dismissed
Show dismissed
Hide dismissed
d38eb1d
to
20f7bb5
Compare
20f7bb5
to
c8679e7
Compare
c8679e7
to
246d235
Compare
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.
🚀
.plus(duration) | ||
) | ||
).map(bound -> checkFixedPeriod(now, operator, bound)) | ||
.map(it -> it ? Result.success() : Result.<Void>failure("")) |
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.
To me, returning a Result<Boolean>
from this method would feel more natural. There is a semantic difference between the check failing "structurally" (e.g. because malformed right-operand), and failing "semantically" (contract expired). WDYT?
Anyway, there should be some information on the failure:
.map(it -> it ? Result.success() : Result.<Void>failure("")) | |
.map(it -> it ? Result.success() : Result.<Void>failure("Contract expired!")) |
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.
yes, in fact I think I modeled this in an overcomplicated way (I was on a rush focusing to have everything working 😅 ), now I re-refactored in a single "function" class and it works in the same way as before (in case of expiration false
is returned without messages), but maybe a message would be appreciated (e.g. to be appended to the transfer error detail).
...main/java/org/eclipse/edc/connector/controlplane/contract/policy/ContractExpiryFunction.java
Outdated
Show resolved
Hide resolved
...in/java/org/eclipse/edc/protocol/dsp/http/dispatcher/DspHttpRemoteMessageDispatcherImpl.java
Outdated
Show resolved
Hide resolved
...in/java/org/eclipse/edc/protocol/dsp/http/dispatcher/DspHttpRemoteMessageDispatcherImpl.java
Outdated
Show resolved
Hide resolved
@@ -41,10 +42,10 @@ <M extends RemoteMessage, R> void registerMessage(Class<M> clazz, DspHttpRequest | |||
/** | |||
* Registers a {@link Policy} scope to be evaluated for certain types of messages | |||
* | |||
* @param messageClass the message type for which evaluate the policy. | |||
* @param scope the scope to be used. | |||
* @param <M> the message type. |
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.
this will almost certainly trip up the javadoc
task. avoid chevrons ("<"
) in javadoc
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.
looks like javadoc is not angry at it, anyway no problem in removing it
I just fixed the spacing, but the line was already there
import org.eclipse.edc.policy.engine.spi.PolicyValidatorFunction; | ||
import org.eclipse.edc.policy.engine.spi.RuleFunction; | ||
import org.eclipse.edc.policy.engine.spi.PolicyValidatorRule; | ||
import org.eclipse.edc.policy.engine.spi.RulePolicyFunction; |
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 guess i missed this in the other PR review (or actually, I missed the other review :/), but I find the name RulePolicyFunction
a bit ... clunky... How about PolicyRuleFunction
?
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 guess it's because "policy function" can be considered an entity on its own, so this is a PolicyFunction that targets a Rule.
rename makes sense anyway
* @param policyProvider function that extracts the Policy from the message. | ||
* @param <M> the message type. |
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.
@paullatzelsperger , in fact it was already there before, I will not remove it
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.
so long as javadoc
doesn't vomit all over itself i'm fine with it :)
630cab3
to
6ea58d2
Compare
What this PR changes/adds
NOTE: wait for #4542 to be merged before reviewing
Sorry for the big PR but this is quite an impactful change.
Highlights:
PolicyContext
is bound to the stringscope
PolicyContext
hierarchy reflects the scope onePolicyEngine
no more accepts (or better, only methods that been deprecated do)String scope
but it relies on thePolicyContext
hierarchy to filter out validators and functions to be applied.all the other changes are related to that.
note: no breaking changes in the API, as the new interfaces have been set as older ones superinterfaces. Deprecations annotations have been put in place.
Upcoming work:
RuleBindingRegistry
to thePolicyEngine
hierarchy (maybe it could also be inlined in thePolicyEngine
, as it is only used internally by the latter.Why it does that
Briefly state why the change was necessary.
Further notes
List other areas of code that have changed but are not necessarily linked to the main feature. This could be method
signature changes, package declarations, bugs that were encountered and were fixed inline, etc.
Linked Issue(s)
Closes #3511
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.