fix: add missing sql dependency to bom and remove not null constraint #713
Conversation
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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...-sql/src/main/java/org/eclipse/edc/identityhub/store/sql/credentials/SqlCredentialStore.java
Fixed
Show fixed
Hide fixed
...-sql/src/main/java/org/eclipse/edc/identityhub/store/sql/credentials/SqlCredentialStore.java
Fixed
Show fixed
Hide fixed
ndr-brt
force-pushed
the
709-fix-sql-bug
branch
from
65afd2e to
4c7193b
Compare
Contributor
|
Hi @ndr-brt Just a quick note, the rawVc was intentionally set to null, since the issuer should not store the signed VC in the database. |
Member
Author
In fact I discovered that I'm not fixing the bug that I reported but a different one! 🥲 EDIT: done |
ndr-brt
force-pushed
the
709-fix-sql-bug
branch
from
4c7193b to
77706cd
Compare
ndr-brt
changed the title
ndr-brt
force-pushed
the
709-fix-sql-bug
branch
from
28863ef to
3c05baa
Compare
...g/eclipse/edc/issuerservice/credentials/statuslist/bitstring/BitstringStatusListManager.java
Show resolved
Hide resolved
ndr-brt
force-pushed
the
709-fix-sql-bug
branch
from
3c05baa to
69a61f7
Compare
ndr-brt
force-pushed
the
709-fix-sql-bug
branch
from
69a61f7 to
6664ebe
Compare
.../org/eclipse/edc/issuerservice/spi/credentials/statuslist/StatusListCredentialPublisher.java
Show resolved
Hide resolved
paullatzelsperger
approved these changes
Apr 25, 2025
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.
What this PR changes/adds
Removes
NOT NULLconstraint on raw_vc column in sql credential store.Adds
identity-hub-credentials-store-sqlto the sql-bom so the flow can be covered by theDcpIssuanceFlowEndToEndTest, that was still failing.To fix it I had to add a patch in the
BitstringStatusListManager, because otherwise thepublished=trueinformation gets lost.That does the job but in a really ugly and not optimal way, so my proposal would be:
Remove persistance from the
StatusListCredentialPublisherimplementation, because, as the interface is stating, that component should just publish and report the result of the publication, and the persistence could be made by the caller (theBitstringStatusListManager) directly, so in the.composeblock after calling the publisher thepublishedflag could be directly added to the metadata together with thepublicUrlone.@wolf4ood @paullatzelsperger let me know what you think about, I can take care to do this change directly
NOTE: unit tests are broken but I'd prefer to have some discussion about the solution to adopt before going and fixing them
Why it does that
Briefly state why the change was necessary.
Further notes
canHandleandunpublishmethods from theStatusListCredentialPublisheras they are not used yet and it's still not clear how they'll be used, to be reintroduced back when needed with the proper signatureWho will sponsor this feature?
Please @-mention the committer that will sponsor your feature.
Linked Issue(s)
Closes #709
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.