-
Notifications
You must be signed in to change notification settings - Fork 240
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
feat: add pending state to ContractNegotiation
and TransferProcess
#3321
feat: add pending state to ContractNegotiation
and TransferProcess
#3321
Conversation
/** | ||
* Represent a column and its value into a JDBC prepared statement. | ||
* | ||
* @param columnName the name of the column. |
Check notice
Code scanning / CodeQL
Spurious Javadoc @param tags
* Represent a column and its value into a JDBC prepared statement. | ||
* | ||
* @param columnName the name of the column. | ||
* @param value the value of the column, by default it will be '?' but it could be different. |
Check notice
Code scanning / CodeQL
Spurious Javadoc @param tags
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #3321 +/- ##
==========================================
+ Coverage 71.91% 71.98% +0.06%
==========================================
Files 810 810
Lines 16513 16613 +100
Branches 964 965 +1
==========================================
+ Hits 11876 11959 +83
- Misses 4236 4252 +16
- Partials 401 402 +1
☔ View full report in Codecov by Sentry. |
6918aad
to
f3065bd
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.
documentation should be added here and there, LGTM otherwise.
Love the SqlExecuteStatement
BTW 🚀
*/ | ||
public class StateProcessorImpl<T> implements StateProcessor { | ||
@Deprecated(since = "0.1.3") | ||
public class StateProcessorImpl<T> implements Processor { |
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.
is there really any point to deprecating this? It's an internal class, not intended for public use
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.
At least tractus-x is using this for the EDR state machine, I think this deprecation work helps a lot migrating from version to version (because the @deprecated
tag in the javadoc text)
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.
It seems this begs a broader discussion about SPIs, what we should and should not deprecate etc.
...ain/java/org/eclipse/edc/connector/contract/ContractNegotiationDefaultServicesExtension.java
Show resolved
Hide resolved
core/common/state-machine/src/main/java/org/eclipse/edc/statemachine/ProcessorImpl.java
Outdated
Show resolved
Hide resolved
import static java.util.stream.Collectors.joining; | ||
import static org.eclipse.edc.sql.statement.ColumnEntry.standardColumn; | ||
|
||
public class SqlExecuteStatement { |
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.
doc? Maybe SqlModificationStatement
would be a more suitable name, to mirror the SqlQueryStatement
?
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 took the name from the methods in SqlQueryExecutor
: execute
(to modify) and query
(to get), I think it make sense to stay consistent, I'm open to modifications but on both sides eventually
f3065bd
to
f2cf806
Compare
What this PR changes/adds
Adds a
pending
field to theStateEntity
extension classes (ContractNegotiation
andTransferProcess
), and the possibility to enable it based on ax extensiblePendingGuard
. (seestate_machine.md
docs for details)Why it does that
This is the first step to enable "external interactions", that is the foundation for features such as the "counter offer" flow.
Further notes
Builder
basedProcessorImpl
that will supersedStateProcessorImpl
TransferProcessDefaultServicesExtension
andContractNegotiationDefaultServicesExtension
, to tidy up a littleSqlExecuteStatement
that helps in building upINSERT
andUPDATE
sql statements without having to fight againstString.format
stuff. Ref: refactor: avoid manual SQL statements definition #3322Linked Issue(s)
Closes #3308
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.