Adding user_tag table to facilitate user restricted application features.

[ghost]

• pinning allowlist and user account storage limits can be managed by this single table without the need for one-off tables e.g. pinning_authorization.
• this allows the admin.storage site to easily manage tags.

nft pr here

[LeslieOA]

@trigramdev9 This makes a lot of sense and makes extending feature access trivial in the future.
I guess "tag" is usefully more generic than auth or permissions in this case.
Would we need a massive enum of all potential options for tag_value also, or would that be excessive?

[ghost]

Would we need a massive enum of all potential options for tag_value also, or would that be excessive?

That's a great question. I asked dchoi on slack if there is a finite set of values (i.e. for storage limits) and he said no, so I'm somewhat assuming it may be more flexible to have tag_value remain a text field.

[dashcraft]

Identical, LGTM!

[vasco-santos]

What value will exist for "Pinning"? Empty?

[ghost]

yea. are you thinking null values would consume too much space in the db? the alternative is a user_tag value table we could join on.

[adamalton]

I think my answer above provides a suggested resolution to this question.

[vasco-santos]

not concerned on space, just on consumer expectations to be different for each type of tag. Which would leave us with hardcoded logic for each context. That's why I suggested the type column, which seems we are aligned :)

[vasco-santos]

What happens if tag deleted and then created for same user? In other words, should we have updated_at?

[flea89]

I wonder if it's better to keep it this way to allow to have a "history".
So we would have a new row for every state change, what do you think?

[vasco-santos]

@flea89
From https://www.notion.so/pl-strflt/Enforcing-user-storage-capacity-limits-a29aa59f28484842bea2721f5982234a#c7c52385776f49148c28d4fbdede6f1b we were already keeping a history table, that is basically interacted by admin dashboard. It also has "reason" for blocking, or unblocking. With that in mind, I think we should just have the latest in this table, which is the table that controls the product flows authorization. Any thing that I am missing?

[flea89]

Yeah, you're right but we'd be tracking 2 different things.
This table would give you the history trail on how how a user tag as evolved ( ie how your limits have increased) which is different from block history.
So I don't see any harm in keeping the trail here as well and, as you said, take the latest (not deleted) to define the outcome.

[vasco-santos]

I think that is fine, have no strong preference.

But, thinking better about updated_at individually, we should still have it as tag_value can be updated. Or were you considering deleting and adding a new one?

[flea89]

If we want history we would have to have to delete (update the deleted_at) and add a new line indeed.
I'm not too opinionated as well, but I think, especially from a reporting perspective, it's useful information to keep the history.
Do you see any problem with that?

[ghost]

@vasco-santos i believe admin would delete then add a new row to preserve a history. updated_at muddies history a bit.

[ghost]

actually on further thought i'm gonna add updated_at.

[adamalton]

@trigramdev9 what's your thinking behind adding updated_at? I thought that the inserted_at and deleted_at combo to give us history was quite neat. Interested to know what's swayed you the other way.

[vasco-santos]

if we want to go with delete and create new one, sounds good to not have updated_ at

[vasco-santos]

I am a bit concerned here. Why a text with "1TB" string?

Two things here:

• text for being a generic value is ok, but I would prefer to see a type enum with the type the value is and a text value that you can parse as stated by type
• We stored user used storage as bytes number in the DB. We should follow same pattern here and use bytes number value.

With above, I would expect the admin dashboard to be able to receive and parse tag values such as 1TB, but convert it to bytes instead. Everything would be handled in bytes within API logic instead of conversions for every single validation of used storage

[ghost]

Given that user_tag is meant to be a very general table, it follows that potential values for tags may range from all sorts of different data types. PINNING doesn't need a value, STORAGE_LIMIT uses bytes as you mentioned: but what about future tags? To make the tag value bytes assumes that all future tags will have byte values. Also thinking about this from an admin perspective, I wonder if an admin user will want to type 1000000000000 when they really mean 1TB?

If I understand you correctly though @vasco-santos , I think your main concern is that the applications will have to convert string values to bytes for storage limit logic. Is that correct?

[adamalton]

We (@LeslieOA, @flea89 and I) discussed this earlier today (but before your comment was added @trigramdev9). I'll summarise our thoughts…

We interpreted @vasco-santos's comment as suggesting that we should add an additional column to denote the data type which is being stored in the tag_value column. And we agreed that that seems like a good idea. So then the table and enums would look like:

CREATE TYPE user_tag_type AS ENUM
(
  'PSA_ENABLED',
  'STORAGE_LIMIT_BYTES'
  -- others can be added in future
);

CREATE TYPE user_tag_value_type AS ENUM
(
  'bool',
  'int'
  'float'
  -- Potentially other types, as the application may require in future
);

CREATE TABLE IF NOT EXISTS public.user_tag
(
  tag                user_tag_type          NOT NULL,
  value_type     user_tag_value_type
  value          TEXT
  -- other fields (id, user_id, inserted_at, deleted_at) as before
);

So then, for example, the storage limit use case would be like:

• tag = "STORAGE_LIMIT_BYTES" (from the enum)
• value_type = "int" (from the enum)
• value = "1099511627776" (text)

And the pinning authorization use case would be like:

• tag = "PSA_ENABLED" (from the enum)
• value_type = "bool" (from the enum)
• value = "true" (text)

@trigramdev9, re. your point about an admin using having to type 1000...0000 instead of 1TB: I'm thinking that this should be solved in the UI level, rather than at the DB level. (Which I think is what @vasco-santos is suggesting in his last paragraph above..?) So each part of the application (the admin UI, the upload API, etc) can get the raw value and then use/present it as necessary.

One minor note, is that in my DB schema above, I've omited the tag_ prefix from the value_type and value columns, just because the table is already called user_tag, so they're hopefully unnecessary (but happy to be defeated on this).

Thoughts on the above? Good? Objections? :-)

[jsdevel]

@adamalton i see. yea, i like your proposal and feel it will fit in and be generic enough for all use cases i can think of moving forward. can you see any downsides to this approach? i do wonder if being too general here is a limiting factor, but for now it seems good. not sure how i missed the proposal from @vasco-santos to have another column to denote the type but that makes perfect sense to me.

[adamalton]

can you see any downsides to this approach?

@jsdevel I think that this approach gives us the best balance that we're going to get between flexible/generic and not too crazy. The only thing that ran through my mind was that casting values from text to their declared type (e.g. parseInt(text_value)) feels like it's veering a little bit towards eval(untrusted_value_from_db)! But I think it's safely a good distance away from that yet!

[vasco-santos]

Thanks @adamalton to put that code snippet together! That was exactly my thoughts