chore(migrations)!: connections 0.3.0 migration script and tests

[amanji]

This PR introduces changes to how connection types are handled in connection records:

• Connection types were previously stored in tags under the connectionType label. Connection types are now directly part of the record.
• Methods to add, update and remove types in the connection service now apply directly to the record instead of tags.
• The connectionType property is now pluralized to connectionTypes to reflect the fact that the property accepts an Array of connection types.
• Methods in the connection API and service have also been pluralized accordingly.
• Relevant tests have been updated to reflect changes to the connection record, API and service and have been added for the connection migration script from AFJ 0.2 to 0.3.

Related Issues

#1155

[codecov-commenter]

Codecov Report

Merging #1177 (08dcc0e) into main (ff6293c) will increase coverage by 1.30%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1177      +/-   ##
==========================================
+ Coverage   87.40%   88.71%   +1.30%     
==========================================
  Files         706      707       +1     
  Lines       16535    16562      +27     
  Branches     2804     2806       +2     
==========================================
+ Hits        14453    14693     +240     
+ Misses       1940     1755     -185     
+ Partials      142      114      -28     

Impacted Files	Coverage Δ
...rc/storage/migration/updates/0.2-0.3/connection.ts	100.00% <100.00%> (ø)
...ore/src/storage/migration/updates/0.2-0.3/index.ts	100.00% <100.00%> (ø)
packages/core/src/storage/BaseRecord.ts	100.00% <0.00%> (+9.52%)	⬆️
...s/src/signature-suites/BbsBlsSignatureProof2020.ts	81.45% <0.00%> (+73.38%)	⬆️
...es/bbs-signatures/src/Bls12381g2SigningProvider.ts	96.29% <0.00%> (+81.48%)	⬆️
...atures/src/signature-suites/BbsBlsSignature2020.ts	87.28% <0.00%> (+83.05%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[TimoGlastra]

One thing that I noticed during this PR review, and maybe as we are already writing a migration script we could update is that a) the property is connectionType and not connectionTypes. It was first singular and I think we forgot to update the naming to plural. Second is that it is only stored as tag. As tags are not always nice to work with, my preference always goes to adding it as either a value to the connection record itself (and add it as a tag in the getTags() method in the connection record). or setting it in the metadata, where we can still include it as a tag for the record.

Would you be willing to include those changes in this PR? So:

• Add an _internal/connectionTypes value to the record metadata with the connectionTypes property
• rename the connectionType to connectionTypes in the DefaultConnectionTags
• Update all methods that were using the connectionType tags to rather set the value on the record (connection service, mediator module, etc..)

And update the migration script to (i think the way you retrieve all connections, is already set up to do this):

• set the connectionTypes value on the connection record based on the connectionType tag
• remove the connectionType tag from the record (connectionRecord.setTag('connectionType', undefined))

Otherwise I can look at making that additional PR, but I think it would make it easier to work with the connection types

[genaris]

One thing that I noticed during this PR review, and maybe as we are already writing a migration script we could update is that a) the property is connectionType and not connectionTypes. It was first singular and I think we forgot to update the naming to plural. Second is that it is only stored as tag. As tags are not always nice to work with, my preference always goes to adding it as either a value to the connection record itself (and add it as a tag in the getTags() method in the connection record). or setting it in the metadata, where we can still include it as a tag for the record.

Would you be willing to include those changes in this PR? So:

• Add an _internal/connectionTypes value to the record metadata with the connectionTypes property
• rename the connectionType to connectionTypes in the DefaultConnectionTags
• Update all methods that were using the connectionType tags to rather set the value on the record (connection service, mediator module, etc..)

And update the migration script to (i think the way you retrieve all connections, is already set up to do this):

• set the connectionTypes value on the connection record based on the connectionType tag
• remove the connectionType tag from the record (connectionRecord.setTag('connectionType', undefined))

Otherwise I can look at making that additional PR, but I think it would make it easier to work with the connection types

While I'm fine with current naming, I think it makes sense to rename it to connectionTypes but finally by reading your comment it was not clear to me if you are willing to add the property to the record itself (like connectionId, threadId, imageUrl, etc) or to its metadata. In my opinion, working with metadata is as complicated as working with tags (and maybe more). The advantage of it is you can set an arbitrary object there, which is not required in this case.

Adding connectionTypes as a direct ConnectionRecord property could make it easier to work with, mostly when we are in the need of updating several ConnectionRecord properties at once (I don't see a common use case yet but it might be). For current use case I think it's not needed because usually, as an an end user, for setter we always need to call ConnectionRepository.update() afterwards so that's why the addConnectionTypes / removeConnectionTypes API/Service methods are convenient to deal with it even if the internals are not so straightforward. The getter, as you said in another comment is not so complicated anyway (just record.getTags().connectionType or the Api method getConnectionTypes(record).

[amanji]

@genaris @TimoGlastra What if we employ the "crawl, walk, run" strategy here as it seems that this next release is time sensitive? How about I at least rename the metadata correctly and in a subsequent release we can discuss moving the data directly into the record?

[TimoGlastra]

@genaris @TimoGlastra What if we employ the "crawl, walk, run" strategy here as it seems that this next release is time sensitive? How about I at least rename the metadata correctly and in a subsequent release we can discuss moving the data directly into the record?

Yes makes sense. I was overly complicating things...

[TimoGlastra]

One thing that I noticed during this PR review, and maybe as we are already writing a migration script we could update is that a) the property is connectionType and not connectionTypes. It was first singular and I think we forgot to update the naming to plural. Second is that it is only stored as tag. As tags are not always nice to work with, my preference always goes to adding it as either a value to the connection record itself (and add it as a tag in the getTags() method in the connection record). or setting it in the metadata, where we can still include it as a tag for the record.
Would you be willing to include those changes in this PR? So:

• Add an _internal/connectionTypes value to the record metadata with the connectionTypes property
• rename the connectionType to connectionTypes in the DefaultConnectionTags
• Update all methods that were using the connectionType tags to rather set the value on the record (connection service, mediator module, etc..)

And update the migration script to (i think the way you retrieve all connections, is already set up to do this):

• set the connectionTypes value on the connection record based on the connectionType tag
• remove the connectionType tag from the record (connectionRecord.setTag('connectionType', undefined))

Otherwise I can look at making that additional PR, but I think it would make it easier to work with the connection types

While I'm fine with current naming, I think it makes sense to rename it to connectionTypes but finally by reading your comment it was not clear to me if you are willing to add the property to the record itself (like connectionId, threadId, imageUrl, etc) or to its metadata. In my opinion, working with metadata is as complicated as working with tags (and maybe more). The advantage of it is you can set an arbitrary object there, which is not required in this case.

Adding connectionTypes as a direct ConnectionRecord property could make it easier to work with, mostly when we are in the need of updating several ConnectionRecord properties at once (I don't see a common use case yet but it might be). For current use case I think it's not needed because usually, as an an end user, for setter we always need to call ConnectionRepository.update() afterwards so that's why the addConnectionTypes / removeConnectionTypes API/Service methods are convenient to deal with it even if the internals are not so straightforward. The getter, as you said in another comment is not so complicated anyway (just record.getTags().connectionType or the Api method getConnectionTypes(record).

@genaris you're right. This comment from me was a lot of thoughts going in all directions. Let's rename to plural naming now and maybe add it to the record in a later release. I'm fine with adding it to the record directly, and can understand the complexity of the record metadata. I think I'm not a fan of using the tags as a way to store data. I see it as away to query data, and we should call getTags() as little as possible. If we need to use getTags(), most of the times I think the data should actually be stored in the record (either directly or in metadata), and automatically set as a tag in the getTags() method.

[genaris]

@genaris you're right. This comment from me was a lot of thoughts going in all directions. Let's rename to plural naming now and maybe add it to the record in a later release. I'm fine with adding it to the record directly, and can understand the complexity of the record metadata. I think I'm not a fan of using the tags as a way to store data. I see it as away to query data, and we should call getTags() as little as possible. If we need to use getTags(), most of the times I think the data should actually be stored in the record (either directly or in metadata), and automatically set as a tag in the getTags() method.

I think this is a very good point, as maybe we were too implementation-specific when deciding to use tags, as the original need solved by connectionType was to filter out Mediator connections and the we generalized it. I agree on handling it later in a separate PR/release.

[amanji]

So I didn't see these additional comments before proceeding, but I've gone ahead and added the connectionTypes directly to the record and updated the methods accordingly. I have added a metadata type (which is not being used at this point) so I can remove that change to keep the PR clean. Let me know your thoughts

[TimoGlastra]

This looks great @amanji. Left some small final remarks