chain: Refactor ObjectType type #75

Merged
fyrchik merged 1 commit from aarifullin/policy-engine:fix/refactor_cnr_obj_type into master 2024-09-04 19:51:23 +00:00
Member

Probably, ObjectType was mistaken for frostfs object types, but actually policy-engine knows (or should know) nothing about container and objects (the exception is schema constants).

  • Rename ObjectType to Kind;
  • Rename Object field in Condition to ConditionKind;
  • Regenerate easy-json marshalers/unmarshalers;
  • Remove ContainerResource, ContainerRequest constants as they
    should not be used at all;
  • Fix unit-tests.
Probably, `ObjectType` was mistaken for frostfs object types, but actually policy-engine knows (or should know) nothing about container and objects (the exception is schema constants). * Rename `ObjectType` to `Kind`; * Rename `Object` field in `Condition` to `ConditionKind`; * Regenerate easy-json marshalers/unmarshalers; * Remove `ContainerResource`, `ContainerRequest` constants as they should not be used at all; * Fix unit-tests.
aarifullin requested review from acid-ant 2024-05-07 14:36:07 +00:00
aarifullin requested review from storage-core-committers 2024-05-07 14:36:07 +00:00
aarifullin requested review from storage-core-developers 2024-05-07 14:36:09 +00:00
fyrchik reviewed 2024-05-07 15:16:22 +00:00
@ -71,1 +70,4 @@
Op ConditionType
// Object specifies for which data this condition will be applied to (Resource, Request).
// Object should be not confused with the same name entity from FrostFS.
Object ObjectType
Owner

It definitely causes confusion. What about renaming it to Kind?
The type should also be named Kind and constants KindResource, KindRequest.

It definitely causes confusion. What about renaming it to `Kind`? The type should also be named `Kind` and constants `KindResource`, `KindRequest`.
Author
Member

Yeah, Kind really sounds good 👍

Yeah, `Kind` really sounds good 👍
Author
Member

But this breaks backward compatibility - "old" chains with Object won't be parsed

But this breaks backward compatibility - "old" chains with `Object` won't be parsed
Owner

Why, though? The binary format is unchanged, besides removed items, which is unrelated to naming.

Btw if removing these constant breaks backward-compatibility with v0.38.4 version of node, we shouldn't do it.

Why, though? The binary format is unchanged, besides removed items, which is unrelated to naming. Btw if removing these constant breaks backward-compatibility with v0.38.4 version of node, we shouldn't do it.
Owner

Breaking compatibility in frostfs-cli is fine, though.

Breaking compatibility in frostfs-cli is fine, though.
aarifullin force-pushed fix/refactor_cnr_obj_type from 8459fe38d1 to f859839a7d 2024-05-08 07:25:06 +00:00 Compare
dstepanov-yadro approved these changes 2024-05-08 08:24:59 +00:00
dstepanov-yadro approved these changes 2024-05-13 08:03:10 +00:00
acid-ant approved these changes 2024-05-13 08:44:08 +00:00
aarifullin force-pushed fix/refactor_cnr_obj_type from f859839a7d to 7b876453ac 2024-05-13 10:11:11 +00:00 Compare
aarifullin changed title from chain: Factor out condition object types for container to chain: Refactor ObjectType type 2024-05-13 10:11:25 +00:00
aarifullin force-pushed fix/refactor_cnr_obj_type from 7b876453ac to dbf41eb77f 2024-05-13 10:13:13 +00:00 Compare
aarifullin requested review from dstepanov-yadro 2024-05-13 10:15:52 +00:00
aarifullin requested review from acid-ant 2024-05-13 10:15:53 +00:00
aarifullin requested review from storage-services-committers 2024-05-13 10:16:07 +00:00
aarifullin requested review from storage-services-developers 2024-05-13 10:16:07 +00:00
aarifullin force-pushed fix/refactor_cnr_obj_type from dbf41eb77f to 43f1ac9851 2024-05-13 10:55:31 +00:00 Compare
fyrchik reviewed 2024-05-13 10:59:47 +00:00
@ -73,2 +71,2 @@
Key string
Value string
Op ConditionType
ConditionKind ConditionKindType
Owner

It is ok for a type name, but why do we use Condition prefix in the field name? It leads to Condition.ConditionKind full name and some verbosity in JSON format.
Also, Kind is somewhat synonymous with Type.

So what about Kind ConditionKind?

It is ok for a type name, but why do we use `Condition` prefix in the field name? It leads to `Condition.ConditionKind` full name and some verbosity in JSON format. Also, `Kind` is somewhat synonymous with `Type`. So what about `Kind ConditionKind`?
Author
Member

Your point is absolutely fair but the point is that we've got similar names throuought:

(this is explanation for the rest of reviewers who didn't get the point of this renaming yet)

But I agreed that Condition.ConditionKind really looks ugly. Let's try the solution suggested by you 'Kind ConditionKind`

Your point is absolutely fair but the point is that we've got similar names throuought: https://git.frostfs.info/TrueCloudLab/frostfs-contract/src/commit/db361318009cc9c9df97133c8c2acedad4eb2fd4/policy/policy_contract.go#L14 (this is explanation for the rest of reviewers who didn't get the point of this renaming yet) But I agreed that `Condition.ConditionKind` really looks ugly. Let's try the solution suggested by you 'Kind ConditionKind`
Author
Member

UPD: Renamed to Kind ConditionKind

UPD: Renamed to `Kind ConditionKind`
aarifullin force-pushed fix/refactor_cnr_obj_type from 43f1ac9851 to 84c4872b20 2024-05-13 14:36:34 +00:00 Compare
fyrchik merged commit 84c4872b20 into master 2024-05-13 15:30:32 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
TrueCloudLab/storage-services-committers
TrueCloudLab/storage-services-developers
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/policy-engine#75
No description provided.