feat(backend): update wallet address additional properties #2769

BlairCurrey · 2024-06-07T15:44:49Z

Changes proposed in this pull request

Updates walletAddressUpdate mutation so that additionalProperties can be updated

Context

Checklist

netlify · 2024-06-07T15:45:05Z

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Name	Link
🔨 Latest commit	`c063b93`
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/6671c348e3f41b00083e4d41

BlairCurrey · 2024-06-07T15:46:42Z

packages/backend/src/open_payments/wallet_address/service.ts

+    // Override all existing additional properties if new ones are provided
+    if (additionalProperties && additionalProperties.length) {
+      await WalletAddressAdditionalProperty.query(trx)
+        .where('walletAddressId', id)
+        .delete()
+      await WalletAddressAdditionalProperty.query(trx).insert(
+        additionalProperties.map((prop) => ({
+          walletAddressId: id,
+          ...prop
+        }))
+      )
+    }


Override all existing additional properties if new ones are provided. So it effectively, but not literally, updates existing keys. Doing it this way is simpler - we dont need to add an index for the key, look each one up, check the value, etc. And the result is the same.

BlairCurrey · 2024-06-07T15:52:25Z

packages/backend/src/open_payments/wallet_address/service.ts

      .patchAndFetch(update)
      .withGraphFetched('asset')
      .throwIfNotFound()
+
+    // Override all existing additional properties if new ones are provided
+    if (additionalProperties && additionalProperties.length) {


Does nothing if additionalProperties is []. Perhaps this is wrong though and an empty array should delete them? Since it's more consistent with how we're otherwise deleting non-existant keys. And because there is currently no way to delete everything.

I did it this way originally because I thought it would be too easy to accidentally delete everything, but now I see that this reasoning would also apply to deleting non-existent keys.

I'll let this marinate a bit and welcome feedback, but I think I'm going to change it so that [] overrides the existing (so deleting everything).

I was getting hung-up on feeling like the delete operation in the database is somehow different than updating the publicName, etc. field on wallet address. But its not really from the api consumer perspective - its just overriding the previous value (an array of things) with the new value (a new array of things, empty or otherwise). So it would be quite strange to set additionalProperties to [] and additionalProperties not to be [].

This is a bit of a tricky one. Here is a suggestion:

undefined would ignore doing anything to the additional properties

[] would delete all the additional properties

keys that match existing values, but have their new value is empty, gets deleted

new keys with valid values are inserted

keys with updated values are updated (non-case-sensitive, not sure if this is already expected to the handled by PSQL behaviour?)

Agree on all counts except I'd rather effectively update existing keys for the reasons stated here: #2769 (comment)

What do you think?

Agree with both of the last two messages 👍 @koekiebox @BlairCurrey

koekiebox · 2024-06-12T04:04:05Z

packages/backend/src/graphql/resolvers/wallet_address.test.ts

+          .then((query): UpdateWalletAddressMutationResponse => {
+            if (query.data) {
+              return query.data.updateWalletAddress
+            } else {


Ok, it is nicer that the additional properties has their own section.

koekiebox

Couple of items, otherwise it looks good.

koekiebox · 2024-06-12T04:16:09Z

packages/backend/src/graphql/resolvers/wallet_address.test.ts

+        { key: '', value: '', visibleInOpenPayments: false },
+        { key: 'key', value: '', visibleInOpenPayments: false },
+        { key: '', value: 'val', visibleInOpenPayments: false },
+        { key: 'key', value: 'val', visibleInOpenPayments: false },


I just realized that I haven't thought of duplicating key's for additional properties.
Do we want to:

generate an error and reject the create.

Only use the value for the last matching keys.

That is an interesting case.

I actually think we should let them do it because it doesn't break nor complicate anything on our end and I dont want to assume there is no valid use-case for duplicate keys. Perhaps they want to group by key or something where only some of the values are private to open payments:

// graphql update wallet address input const input = { additionalProperties: [ { key: 'myUserArray', value: 'publicUser1', visibleInOpenPayments: 'true' }, { key: 'myUserArray', value: 'privateUser2', visibleInOpenPayments: 'false' }, { key: 'myUserArray', value: 'publicUser2', visibleInOpenPayments: 'true' }, { key: 'nickname', value: 'someNickname', visibleInOpenPayments: 'true' } ] } // /GET wallet-address response const response = { additionalProperties: [ { key: 'myUserArray', value: 'publicUser1', visibleInOpenPayments: 'true' }, { key: 'myUserArray', value: 'publicUser2', visibleInOpenPayments: 'true' }, { key: 'nickname', value: 'someNickname', visibleInOpenPayments: 'true' } ] } // group values by 'myUserArray' const userArrayValues = response.additionalProperties .filter(obj => obj.key === 'myUserArray') .map(obj => obj.value); // users is ["publicUser1", "publicUser2"]

This is a capability we would remove by disallowing duplicate keys. Do they really want to do this? I have no idea - but I think that's their concern and we shouldn't reduce their power to choose to use it this way, or other un-anticipated ways when it doesn't break nor complicate anything on our end.

If duplicate keys from the bad input they provide is a problem on their end they can deduplicate them however they want and re-use the update mutation.

mkurapov · 2024-06-14T15:26:19Z

packages/backend/src/open_payments/wallet_address/service.ts

 ): Promise<WalletAddress | WalletAddressError> {
+  const trx = await WalletAddress.startTransaction()
  try {
    const update: UpdateInput = { publicName }
    const walletAddress = await WalletAddress.query(deps.knex)


not a big deal but should we also use the trx here?

I suppose probably so... and more importantly the underlying IncomingPayment.patchin deactivateOpenIncomingPaymentsByWalletAddress (called a few lines below this). Added 8f64e7c

mkurapov · 2024-06-14T15:29:12Z

packages/backend/src/open_payments/wallet_address/service.ts

+        additionalProperties.map((prop) => ({
+          walletAddressId: id,
+          ...prop
+        }))


just in case I think we should "clean" the values passed into this. Would probably best to have this "cleaning" made into a fn and also used during create in L150-159.

Let me know if you have anything else in mind, but I made a fn that takes the existing check in create (that key/value arent empty) and added a step to trim them to get rid of whitespace. Does that cover what you had in mind?

7c21e2d

I guess this suggestion kinda veers into the suggestion to de-duplicate #2769 (comment)

I guess I still prefer to let the caller handle that and not enforce it but I guess if we do this would be the place. Not sure if that's something you had in mind or not.

mkurapov · 2024-06-18T12:18:54Z

packages/backend/src/graphql/resolvers/wallet_address.test.ts

-                    }
-                    url
-                    publicName
+    test('Can create a wallet address with additional properties', async (): Promise<void> => {


we should check the correct properties were returned in the response

updated f73e66d

mkurapov · 2024-06-18T12:20:24Z

packages/backend/src/open_payments/wallet_address/service.ts

      .patchAndFetch(update)
      .withGraphFetched('asset')
      .throwIfNotFound()
+
+    // Override all existing additional properties if new ones are provided
+    if (additionalProperties) {


we should add some tests for these

added: 770c0bd

koekiebox

LGTM! 🎸

mkurapov

LGTM ✅

feat(backend): update additional properties

f81db0e

github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. pkg: frontend Changes in the frontend package. type: source Changes business logic pkg: mock-ase pkg: mock-account-service-lib labels Jun 7, 2024

BlairCurrey commented Jun 7, 2024

View reviewed changes

BlairCurrey added 2 commits June 7, 2024 12:06

chore: rm unecessary test.each

a4ea985

test(backend): getAdditionalProperties resolver for null state

7d74b7e

BlairCurrey requested review from koekiebox, njlie, mkurapov and sabineschaller June 7, 2024 16:27

chore: rm dev comment

87c319f

BlairCurrey removed request for sabineschaller and njlie June 11, 2024 14:42

koekiebox reviewed Jun 12, 2024

View reviewed changes

koekiebox requested changes Jun 12, 2024

View reviewed changes

BlairCurrey added 2 commits June 12, 2024 11:13

feat: allow setting additionalProperties to []

5b4c5bd

Merge branch 'main' into bc/2740/update-wa-additional-properties

aa3df34

mkurapov reviewed Jun 14, 2024

View reviewed changes

BlairCurrey added 2 commits June 17, 2024 09:43

feat: clean additional properties

7c21e2d

fix: use trx

8f64e7c

BlairCurrey requested review from mkurapov and koekiebox June 17, 2024 14:06

mkurapov reviewed Jun 18, 2024

View reviewed changes

Merge branch 'main' into bc/2740/update-wa-additional-properties

76bed7f

BlairCurrey added 2 commits June 18, 2024 12:26

test(backend): add additional properties assertions

f73e66d

test(backend): add update additional properties cases

770c0bd

BlairCurrey requested a review from mkurapov June 18, 2024 17:17

fix(backend): lint

c063b93

koekiebox approved these changes Jun 18, 2024

View reviewed changes

mkurapov approved these changes Jun 18, 2024

View reviewed changes

BlairCurrey merged commit 3bd9669 into main Jun 19, 2024
42 checks passed

BlairCurrey deleted the bc/2740/update-wa-additional-properties branch June 19, 2024 02:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(backend): update wallet address additional properties #2769

feat(backend): update wallet address additional properties #2769

BlairCurrey commented Jun 7, 2024

netlify bot commented Jun 7, 2024 •

edited

Loading

BlairCurrey Jun 7, 2024 •

edited

Loading

BlairCurrey Jun 7, 2024

BlairCurrey Jun 7, 2024 •

edited

Loading

koekiebox Jun 12, 2024

BlairCurrey Jun 12, 2024

mkurapov Jun 14, 2024

koekiebox Jun 12, 2024

koekiebox left a comment

koekiebox Jun 12, 2024

BlairCurrey Jun 12, 2024 •

edited

Loading

mkurapov Jun 14, 2024

BlairCurrey Jun 17, 2024 •

edited

Loading

mkurapov Jun 14, 2024

BlairCurrey Jun 17, 2024

BlairCurrey Jun 17, 2024

mkurapov Jun 18, 2024

BlairCurrey Jun 18, 2024

mkurapov Jun 18, 2024

BlairCurrey Jun 18, 2024

koekiebox left a comment

mkurapov left a comment

feat(backend): update wallet address additional properties #2769

feat(backend): update wallet address additional properties #2769

Conversation

BlairCurrey commented Jun 7, 2024

Changes proposed in this pull request

Context

Checklist

netlify bot commented Jun 7, 2024 • edited Loading

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

BlairCurrey Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Jun 7, 2024 • edited Loading

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

koekiebox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlairCurrey Jun 17, 2024 • edited Loading

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

Choose a reason for hiding this comment

koekiebox left a comment

Choose a reason for hiding this comment

mkurapov left a comment

Choose a reason for hiding this comment

netlify bot commented Jun 7, 2024 •

edited

Loading

BlairCurrey Jun 7, 2024 •

edited

Loading

BlairCurrey Jun 7, 2024 •

edited

Loading

BlairCurrey Jun 12, 2024 •

edited

Loading

BlairCurrey Jun 17, 2024 •

edited

Loading