feat(core)!: metadata on records #505

berendsliedrecht · 2021-10-26T09:18:22Z

BREAKING CHANGE: credentialRecord.credentialMetadata has been replaced by credentialRecord.metadata.

Adds metadata to the baserecord
Does not do any persistency and should be done manually after updating the metadata no the record.
Can be used by framework developers and consumers.

Maintainers: Before merging and squashing make sure to copy the BREAKING CHANGE entry to the commit message field, this is needed for automatic tracking of breaking changes

Signed-off-by: Berend Sliedrecht <[email protected]>

codecov-commenter · 2021-10-26T12:10:54Z

Codecov Report

Merging #505 (162b7c9) into main (336baa0) will increase coverage by 0.06%.
The diff coverage is 94.87%.

@@            Coverage Diff             @@
##             main     #505      +/-   ##
==========================================
+ Coverage   86.42%   86.49%   +0.06%     
==========================================
  Files         266      267       +1     
  Lines        5733     5762      +29     
  Branches      923      932       +9     
==========================================
+ Hits         4955     4984      +29     
  Misses        777      777              
  Partials        1        1

Impacted Files	Coverage Δ
packages/core/src/storage/BaseRecord.ts	`83.87% <83.33%> (+8.87%)`	⬆️
...modules/credentials/repository/CredentialRecord.ts	`92.18% <100.00%> (+0.95%)`	⬆️
.../modules/credentials/services/CredentialService.ts	`95.74% <100.00%> (-0.04%)`	⬇️
packages/core/src/storage/Metadata.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 336baa0...162b7c9. Read the comment docs.

TimoGlastra

Looks good @blu3beri. Only thing I'm not sure about is the checks in the credential record constructor

packages/core/src/storage/Metadata.ts

TimoGlastra · 2021-10-26T15:11:19Z

packages/core/src/modules/credentials/repository/CredentialRecord.ts

+      if (props.metadata) {
+        if (props.metadata.requestMetadata) {
+          this.metadata.set('requestMetadata', props.metadata.requestMetadata)
+        }
+        if (props.metadata.schemaId) {
+          this.metadata.add('indyCredentialMetadata', {
+            schemaId: props.metadata.schemaId,
+          })
+        }
+        if (props.metadata.credentialDefinitionId) {
+          this.metadata.add('indyCredentialMetadata', {
+            credentialDefinitionId: props.metadata.credentialDefinitionId,
+          })
+        }
+      }


We haven't reached v0.1.0 yet so maybe it would be easier to just see this as a breaking change

How would you propose to do this? Some part of the framework (in credentialService.ts) is dependent on these fields.

I would just let them call the metadata themselves

const record = new CredentialRecord({ /** config props */ }) record.metadata.set('indyCredentialMetadata', { schemaId: '', credentialDefinitionId: '' })

It may be wise to increment our version in some manner to indicate a breaking change here, since this could cause fairly significant issues for existing systems? Is there harm in utilizing @blu3beri's original mechanism until a later date?

I think @blu3beri's approach wouldn't work out of the box. The constructor is not called with properties when a record is retrieved from storage. We could add a custom transformer, but I'd rather only take these things into account between stable releases.

Maybe we can look at writing transform scripts. I think we should be very cautious with adding these if statements to the codebase. I don't want to be stuck with this piece of code for eternity. We can never assume when all records are transformed. We're going to have a lot more breaking changes going forward that I'll rather solve by writing update scripts. I'd day that is a reasonable for pre-1.0 releases

Maybe we can keep it in here till 0.1.0. Then for 0.1.0 we provide scripts, and for 0.2.0 we again provide a script for the transformation.

Thoughts?

Maybe I am misunderstanding your point, but right now we set this data in the constructor already and the credentialService, line 612 and 627, relay on these fields. It will even error out when we do not have these fields in the metadata.

I am okay with leaving this out, but how do you propose we handle that error case?

Ah okay we were talking about two different things here. For your point @blu3beri, I suggest we remove the metadata from the constructor and require it to be set from using the record.metadata.set afterwards.

What I was mainly thinking about here is the persistence. The metadata on the current credential records has a different structure than the metadata we're adding in this PR. My thinking was that we add something like this migration script called 0.1.0-alpha.291-0.1.0-alpha.292.ts

export async function migrate291To292(agent: Agent) { const credentialRecords = await agent.credentials.getAll(); const credentialRepository = await agent.injectionContainer.resolve( CredentialRepository ); for (const credentialRecord of credentialRecords) { const data = credentialRecord.metadata.data; const metadata = MetaData(); if (data.requestMetadata) { metadata.set("indyRequestMetadata", data.requestMetadata); } if (data.schemaId) { this.metadata.add("indyCredentialMetadata", { schemaId: data.schemaId, }); } if (data.credentialDefinitionId) { this.metadata.add("indyCredentialMetadata", { credentialDefinitionId: data.credentialDefinitionId, }); } credentialRecord.metadata = metadata; await credentialRepository.update(credentialRecord); } }

Ahh okay. I am in favour of the migration scripts, but for the 0.1.0 and so on. 👍

Yeah I'd be in favor of migration scripts, as that'd be very nice for breaking changes. We could either tackle that now or at a 0.1.0 release, as you mentioned. :)
I'll admit I'm not entirely sure how that should be handled from a mobile app perspective, but that can be figured out. :)

packages/core/src/storage/Metadata.ts

packages/core/src/modules/credentials/services/CredentialService.ts

packages/core/src/storage/BaseRecord.ts

packages/core/src/storage/Metadata.ts

TimoGlastra · 2021-10-31T11:49:21Z

packages/core/src/modules/credentials/repository/CredentialRecord.ts

+      if (props.metadata) {
+        if (props.metadata.requestMetadata) {
+          this.metadata.set('requestMetadata', props.metadata.requestMetadata)
+        }
+        if (props.metadata.schemaId) {
+          this.metadata.add('indyCredentialMetadata', {
+            schemaId: props.metadata.schemaId,
+          })
+        }
+        if (props.metadata.credentialDefinitionId) {
+          this.metadata.add('indyCredentialMetadata', {
+            credentialDefinitionId: props.metadata.credentialDefinitionId,
+          })
+        }
+      }


Ah okay we were talking about two different things here. For your point @blu3beri, I suggest we remove the metadata from the constructor and require it to be set from using the record.metadata.set afterwards.

What I was mainly thinking about here is the persistence. The metadata on the current credential records has a different structure than the metadata we're adding in this PR. My thinking was that we add something like this migration script called 0.1.0-alpha.291-0.1.0-alpha.292.ts

export async function migrate291To292(agent: Agent) { const credentialRecords = await agent.credentials.getAll(); const credentialRepository = await agent.injectionContainer.resolve( CredentialRepository ); for (const credentialRecord of credentialRecords) { const data = credentialRecord.metadata.data; const metadata = MetaData(); if (data.requestMetadata) { metadata.set("indyRequestMetadata", data.requestMetadata); } if (data.schemaId) { this.metadata.add("indyCredentialMetadata", { schemaId: data.schemaId, }); } if (data.credentialDefinitionId) { this.metadata.add("indyCredentialMetadata", { credentialDefinitionId: data.credentialDefinitionId, }); } credentialRecord.metadata = metadata; await credentialRepository.update(credentialRecord); } }

Signed-off-by: Berend Sliedrecht <[email protected]>

TimoGlastra

Nice!

JamesKEbert · 2021-11-01T21:45:40Z

So, just to confirm, this would have a breaking change at this point still, right? I have some cases that would need a way to migrate--so this would be a spot that would need a migration script, right?

berendsliedrecht · 2021-11-02T07:46:09Z

So, just to confirm, this would have a breaking change at this point still, right? I have some cases that would need a way to migrate--so this would be a spot that would need a migration script, right?

Yes, a migration script from all the previous versions to this one is required. @TimoGlastra gave one in the comments above that should, or maybe with some minor tweaking.

Signed-off-by: Berend Sliedrecht <[email protected]>

TimoGlastra · 2021-11-04T15:22:26Z

Decision made on WG call: Use @Transformer for now to transform the record when it is constructed. When moving to 0.1.0 we will add a migration script.

@blu3beri could you make the required changes?

Signed-off-by: Berend Sliedrecht <[email protected]>

packages/core/src/utils/transformers.ts

jakubkoci · 2021-11-08T13:26:13Z

packages/core/tests/credentials-auto-accept.test.ts

+              credentialDefinitionId: credDefId,
+            },
+          },
+        },


It's a bit confusing that we have metadata.data.indyRequestMetadata? Wouldn't be more straightforward to have metadata.indyRequest?

The data object is there just for structure. My design was that the user could not easily call metadata.indyRequestMetdata and do it through the get,set, etc. Now, the data does not remove this, but I do think it promotes the usage of the functions to get values.

renaming indyRequestMetdata to indyRequest is okay with me. It does seem a bit too verbose now that I think about it.

I see. I think it would be more concise to use just indyRequest instead of indyRequestMetdata at least.

jakubkoci · 2021-11-08T13:33:29Z

What if a developer creates some metadata:

credentialRecord.metadata.set('something', { a: 1 })
credentialRecord.metadata.set('something else', { b: 2 })

And then decides to remove all of them:

Object.keys(credentialRecord.metadata.data).forEach(key => credentialRecord.metadata.delete(key))

Is it possible to do so? Would that delete also internal metadata?

berendsliedrecht · 2021-11-08T14:46:39Z

Is it possible to do so? Would that delete also internal metadata?

Yes that is possible and it would also delete the internal metadata.

Signed-off-by: Berend Sliedrecht <[email protected]>

jakubkoci · 2021-11-12T11:11:09Z

If I understand correctly, deleting the internal metadata would break the agent, right?

It would be great if we're able to prevent it somehow.

TimoGlastra · 2021-11-12T11:17:24Z

If I understand correctly, deleting the internal metadata would break the agent, right?

It would be great if we're able to prevent it somehow.

I'd say we prefix it with internal/ if you then delete it that would be weird. Nothing prevents the user from overwriting the state of a record and saving that

berendsliedrecht · 2021-11-12T13:17:26Z

If I understand correctly, deleting the internal metadata would break the agent, right?

It would be great if we're able to prevent it somehow.

I agree with @TimoGlastra here. It will always be possible for a framework consumer to modify the data in a record.

If we would, for instance, throw an error when we try to delete a key-value via record.metadata.delete(key) we could still call the underlying functionality which would not error. e.g. delete metadata.data[key].

I completely understand your point, but next to maybe prefixing it, which I am not even sure we should do, but I don't think we should/can do anything about this.

jakubkoci · 2021-11-12T21:17:05Z

I agree with @TimoGlastra here. It will always be possible for a framework consumer to modify the data in a record.

That's not so easy to do. I realized it's therefore not so easy to manipulate with a metadata object.
But if the purpose of metadata is (or will be) to offer the users some space where they can store (create, update, delete) their metadata for a credential record, it's dangerous to mix those "custom" metadata and "framework" metadata into the same object.

jakubkoci

We can merge it, but I still have concerns that mixing custom and framework metadata can cause trouble in the future.

berendsliedrecht · 2021-11-12T21:34:22Z

We can merge it, but I still have concerns that mixing custom and framework metadata can cause trouble in the future.

If you could create an issue for it I will think about the options that are available.

TimoGlastra · 2021-11-15T09:21:42Z

We can merge it, but I still have concerns that mixing custom and framework metadata can cause trouble in the future.

@jakubkoci What would your ideal solution be for this? Add two types of metadata to each record (framework and consumer metadata)?

TimoGlastra · 2021-11-15T09:25:06Z

@blu3beri I think it would be good to prefix internal metadata to avoid collisions. We can then say that you are never allowed to use this prefix yourself. I've seen this pattern used in other libraries. Could you make a follow-up PR @blu3beri (rather quickly cause it's breaking again) to add the prefix? I'd say we prefix it with something like _internal/ (other suggestions also welcome)

berendsliedrecht · 2021-11-15T09:27:49Z

@blu3beri I think it would be good to prefix internal metadata to avoid collisions. We can then say that you are never allowed to use this prefix yourself. I've seen this pattern used in other libraries. Could you make a follow-up PR @blu3beri (rather quickly cause it's breaking again) to add the prefix? I'd say we prefix it with something like _internal/ (other suggestions also welcome)

What would you say about an internal key? like

metadata.set('internal', {foo: 'bar'...})

This way we group ALL of the internal stuff in one object. (This might be considered a downside, that's why I am asking) :)

TimoGlastra · 2021-11-15T09:30:09Z

Yes that could work, but thinking about v2 protocols supporting multiple credential types I think it may be good to e.g. split it metadata per credential type. But that could of course also be nested under the same key. It would however make updating harder (you can't use .add()`)

berendsliedrecht · 2021-11-15T09:43:54Z

Yes that could work, but thinking about v2 protocols supporting multiple credential types I think it may be good to e.g. split it metadata per credential type. But that could of course also be nested under the same key. It would however make updating harder (you can't use .add()`)

Okay I 10000% agree with the fact that it is harder to update. That was the main reason I did not propose this solution earlier. I will create a new pr with the prefix. I think that _internal is good.

berendsliedrecht added 7 commits October 22, 2021 17:24

feat(core): baserecord metadata

4d7f53d

Signed-off-by: Berend Sliedrecht <[email protected]>

feat(core): baserecord metadata

ea0b8ad

Signed-off-by: Berend Sliedrecht <[email protected]>

feat(core): record metadata

9a0c850

Signed-off-by: Berend Sliedrecht <[email protected]>

fix(core): replaced credential metadata

8592977

Signed-off-by: Berend Sliedrecht <[email protected]>

fix(core): refactored to use metadata

a3b269a

Signed-off-by: Berend Sliedrecht <[email protected]>

fix(core): refactored to use metadata

fac66fa

Signed-off-by: Berend Sliedrecht <[email protected]>

test(core): implemented metadata correct in tests

162b7c9

Signed-off-by: Berend Sliedrecht <[email protected]>

berendsliedrecht marked this pull request as ready for review October 26, 2021 12:12

berendsliedrecht requested a review from a team as a code owner October 26, 2021 12:12

TimoGlastra requested changes Oct 26, 2021

View reviewed changes

JamesKEbert reviewed Oct 29, 2021

View reviewed changes

packages/core/src/storage/Metadata.ts Show resolved Hide resolved

TimoGlastra reviewed Oct 31, 2021

View reviewed changes

fix: resolved additional feedback

2e11eb6

Signed-off-by: Berend Sliedrecht <[email protected]>

TimoGlastra approved these changes Nov 1, 2021

View reviewed changes

berendsliedrecht added 2 commits November 2, 2021 11:26

fix: fixed test cases

cf38cc7

Signed-off-by: Berend Sliedrecht <[email protected]>

refactor(core): removed unused variable

06f2dea

Signed-off-by: Berend Sliedrecht <[email protected]>

fix(core): resolved feedback

0c6781c

Signed-off-by: Berend Sliedrecht <[email protected]>

berendsliedrecht requested a review from TimoGlastra November 8, 2021 12:49

TimoGlastra reviewed Nov 8, 2021

View reviewed changes

packages/core/src/utils/transformers.ts Outdated Show resolved Hide resolved

jakubkoci reviewed Nov 8, 2021

View reviewed changes

berendsliedrecht added 2 commits November 8, 2021 16:29

fix(core): resolved additional feedback

98c74ee

Signed-off-by: Berend Sliedrecht <[email protected]>

refactor(core): minor cleanup

7b74aaf

Signed-off-by: Berend Sliedrecht <[email protected]>

berendsliedrecht requested a review from jakubkoci November 11, 2021 19:53

jakubkoci approved these changes Nov 12, 2021

View reviewed changes

berendsliedrecht merged commit c92393a into openwallet-foundation:main Nov 12, 2021

berendsliedrecht deleted the feat/record-metadata branch August 31, 2022 07:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core)!: metadata on records #505

feat(core)!: metadata on records #505

berendsliedrecht commented Oct 26, 2021 •

edited by TimoGlastra

Loading

codecov-commenter commented Oct 26, 2021 •

edited

Loading

TimoGlastra left a comment

TimoGlastra Oct 26, 2021

berendsliedrecht Oct 26, 2021

TimoGlastra Oct 28, 2021

JamesKEbert Oct 29, 2021

TimoGlastra Oct 30, 2021

berendsliedrecht Oct 31, 2021

TimoGlastra Oct 31, 2021 •

edited

Loading

berendsliedrecht Oct 31, 2021

JamesKEbert Nov 1, 2021

TimoGlastra Oct 31, 2021 •

edited

Loading

TimoGlastra left a comment

JamesKEbert commented Nov 1, 2021

berendsliedrecht commented Nov 2, 2021

TimoGlastra commented Nov 4, 2021

jakubkoci Nov 8, 2021

berendsliedrecht Nov 8, 2021

jakubkoci Nov 12, 2021

jakubkoci commented Nov 8, 2021

berendsliedrecht commented Nov 8, 2021

jakubkoci commented Nov 12, 2021

TimoGlastra commented Nov 12, 2021

berendsliedrecht commented Nov 12, 2021

jakubkoci commented Nov 12, 2021 •

edited

Loading

jakubkoci left a comment

berendsliedrecht commented Nov 12, 2021

TimoGlastra commented Nov 15, 2021

TimoGlastra commented Nov 15, 2021

berendsliedrecht commented Nov 15, 2021

TimoGlastra commented Nov 15, 2021

berendsliedrecht commented Nov 15, 2021

feat(core)!: metadata on records #505

feat(core)!: metadata on records #505

Conversation

berendsliedrecht commented Oct 26, 2021 • edited by TimoGlastra Loading

codecov-commenter commented Oct 26, 2021 • edited Loading

Codecov Report

TimoGlastra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoGlastra Oct 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoGlastra Oct 31, 2021 • edited Loading

Choose a reason for hiding this comment

TimoGlastra left a comment

Choose a reason for hiding this comment

JamesKEbert commented Nov 1, 2021

berendsliedrecht commented Nov 2, 2021

TimoGlastra commented Nov 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakubkoci commented Nov 8, 2021

berendsliedrecht commented Nov 8, 2021

jakubkoci commented Nov 12, 2021

TimoGlastra commented Nov 12, 2021

berendsliedrecht commented Nov 12, 2021

jakubkoci commented Nov 12, 2021 • edited Loading

jakubkoci left a comment

Choose a reason for hiding this comment

berendsliedrecht commented Nov 12, 2021

TimoGlastra commented Nov 15, 2021

TimoGlastra commented Nov 15, 2021

berendsliedrecht commented Nov 15, 2021

TimoGlastra commented Nov 15, 2021

berendsliedrecht commented Nov 15, 2021

berendsliedrecht commented Oct 26, 2021 •

edited by TimoGlastra

Loading

codecov-commenter commented Oct 26, 2021 •

edited

Loading

TimoGlastra Oct 31, 2021 •

edited

Loading

TimoGlastra Oct 31, 2021 •

edited

Loading

jakubkoci commented Nov 12, 2021 •

edited

Loading