[#4007] improvement: Use template to reduce Privileges duplicate codes

rich7420 commented 3 months ago

What changes were proposed in this pull request?

make the template to reduce these duplicate codes.

Why are the changes needed?

Currently Privileges.java have more than one thousand duplicate code to instantiation CreateCatalog, AlterCatalog, ...
we can use template to make Privileges.java smaller. Fix: #4007

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

./gradlew test

jerryshao commented 3 months ago

@qqqttt123 @xunliu can you please help to review this?

xunliu commented 3 months ago

hi @rich7420 Thank you for your contributions.

Please add these two functions in the public abstract static class GenericPrivilege<T extends GenericPrivilege<T>>

    @Override
    public boolean equals(Object o) {
      if (this == o) return true;
      if (o == null || getClass() != o.getClass()) return false;
      GenericPrivilege<?> that = (GenericPrivilege<?>) o;
      return condition == that.condition && name == that.name;
    }

    @Override
    public int hashCode() {
      return Objects.hash(condition, name);
    }

This way, it can fix the issue of Assertions.assertEquals(metalake, anotherMetalake);

rich7420 commented 3 months ago

@xunliu ok! I will try this way. Thank you. I appreciate it.

rich7420 commented 3 months ago

@xunliu Can I change the code like this? Because the original way can't compile successfully.

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        // Change here: replaced getClass() != o.getClass() with !(o instanceof GenericPrivilege)
        if (!(o instanceof GenericPrivilege)) return false;
        GenericPrivilege<?> that = (GenericPrivilege<?>) o;
        return condition == that.condition && name == that.name;
    }

    @Override
    public int hashCode() {
        return Objects.hash(condition, name);
    }

rich7420 commented 3 months ago

Sorry , Could someone please help me rerun the test? The code that failed this time is the same as the code that succeeded before.

qqqttt123 commented 3 months ago

Every privilege should return a singleton instance. Because we cached roles, the roles will contain many privileges. If we use the singleton for every privilege. We can save much memory.

rich7420 commented 3 months ago

@qqqttt123 According to the idea, Maybe I can change code like this to add ALLOW_INSTANCE and DENY_INSTANCE. What do you think?

  public static class CreateCatalog extends GenericPrivilege<CreateCatalog> {
    private static final CreateCatalog ALLOW_INSTANCE = new CreateCatalog(Condition.ALLOW, Name.CREATE_CATALOG);
    private static final CreateCatalog DENY_INSTANCE = new CreateCatalog(Condition.DENY, Name.CREATE_CATALOG);

    private CreateCatalog(Condition condition, Name name) {
      super(condition, name);
    }

    /** @return The instance with allow condition of the privilege. */
    public static CreateCatalog allow() {
      return ALLOW_INSTANCE;
    }

    /** @return The instance with deny condition of the privilege. */
    public static CreateCatalog deny() {
      return DENY_INSTANCE;
    }
  }

qqqttt123 commented 3 months ago

@qqqttt123 According to the idea, Maybe I can change code like this to add ALLOW_INSTANCE and DENY_INSTANCE. What do you think?

  public static class CreateCatalog extends GenericPrivilege<CreateCatalog> {
    private static final CreateCatalog ALLOW_INSTANCE = new CreateCatalog(Condition.ALLOW, Name.CREATE_CATALOG);
    private static final CreateCatalog DENY_INSTANCE = new CreateCatalog(Condition.DENY, Name.CREATE_CATALOG);

    private CreateCatalog(Condition condition, Name name) {
      super(condition, name);
    }

    /** @return The instance with allow condition of the privilege. */
    public static CreateCatalog allow() {
      return ALLOW_INSTANCE;
    }

    /** @return The instance with deny condition of the privilege. */
    public static CreateCatalog deny() {
      return DENY_INSTANCE;
    }
  }

Ok for me.

rich7420 commented 3 months ago

@xunliu thank you. I appreciate it!!!

jerqi commented 3 months ago

@qqqttt123 According to the idea, Maybe I can change code like this to add ALLOW_INSTANCE and DENY_INSTANCE. What do you think?

  public static class CreateCatalog extends GenericPrivilege<CreateCatalog> {
    private static final CreateCatalog ALLOW_INSTANCE = new CreateCatalog(Condition.ALLOW, Name.CREATE_CATALOG);
    private static final CreateCatalog DENY_INSTANCE = new CreateCatalog(Condition.DENY, Name.CREATE_CATALOG);

    private CreateCatalog(Condition condition, Name name) {
      super(condition, name);
    }

    /** @return The instance with allow condition of the privilege. */
    public static CreateCatalog allow() {
      return ALLOW_INSTANCE;
    }

    /** @return The instance with deny condition of the privilege. */
    public static CreateCatalog deny() {
      return DENY_INSTANCE;
    }
  }

Ok for me.

This comment seems not be addressed.

rich7420 commented 3 months ago

@qqqttt123 According to the idea, Maybe I can change code like this to add ALLOW_INSTANCE and DENY_INSTANCE. What do you think?

  public static class CreateCatalog extends GenericPrivilege<CreateCatalog> {
    private static final CreateCatalog ALLOW_INSTANCE = new CreateCatalog(Condition.ALLOW, Name.CREATE_CATALOG);
    private static final CreateCatalog DENY_INSTANCE = new CreateCatalog(Condition.DENY, Name.CREATE_CATALOG);

    private CreateCatalog(Condition condition, Name name) {
      super(condition, name);
    }

    /** @return The instance with allow condition of the privilege. */
    public static CreateCatalog allow() {
      return ALLOW_INSTANCE;
    }

    /** @return The instance with deny condition of the privilege. */
    public static CreateCatalog deny() {
      return DENY_INSTANCE;
    }
  }

Ok for me.

This comment seems not be addressed.

Thank you for your feedback. Added static constants ALLOW_INSTANCE and DENY_INSTANCE in each privilege class, These constants are initialized when the class is loaded, ensuring the instances are created only once. What else can I improve on?

jerqi commented 3 months ago

@qqqttt123 According to the idea, Maybe I can change code like this to add ALLOW_INSTANCE and DENY_INSTANCE. What do you think?
  public static class CreateCatalog extends GenericPrivilege<CreateCatalog> {
    private static final CreateCatalog ALLOW_INSTANCE = new CreateCatalog(Condition.ALLOW, Name.CREATE_CATALOG);
    private static final CreateCatalog DENY_INSTANCE = new CreateCatalog(Condition.DENY, Name.CREATE_CATALOG);

    private CreateCatalog(Condition condition, Name name) {
      super(condition, name);
    }

    /** @return The instance with allow condition of the privilege. */
    public static CreateCatalog allow() {
      return ALLOW_INSTANCE;
    }

    /** @return The instance with deny condition of the privilege. */
    public static CreateCatalog deny() {
      return DENY_INSTANCE;
    }
  }
Ok for me.
This comment seems not be addressed.
Thank you for your feedback. Added static constants ALLOW_INSTANCE and DENY_INSTANCE in each privilege class, These constants are initialized when the class is loaded, ensuring the instances are created only once. What else can I improve on?

You don't add static constants ALLOW_INSTANCE and DENY_INSTANCE in the code.

rich7420 commented 3 months ago

@jerqi Sorry , I forgot to push. Thank you for your reply.

rich7420 commented 3 months ago

@qqqttt123 thank you for your reply. I removed that codes.

xunliu commented 3 months ago

hi @rich7420 Please fix CI issue.

BUILD FAILED in 58s
                -····private·static·final·DropCatalog·ALLOW_INSTANCE·=·new·DropCatalog(Condition.ALLOW,·Name.CREATE_CATALOG);
                -····private·static·final·DropCatalog·DENY_INSTANCE·=·new·DropCatalog(Condition.DENY,·Name.CREATE_CATALOG);
                +····private·static·final·DropCatalog·ALLOW_INSTANCE·=
                +········new·DropCatalog(Condition.ALLOW,·Name.CREATE_CATALOG);
                +····private·static·final·DropCatalog·DENY_INSTANCE·=
                +········new·DropCatalog(Condition.DENY,·Name.CREATE_CATALOG);

                 ····private·DropCatalog(Condition·condition,·Name·name)·{
                 ······super(condition,·name);
                @@ -373,8 +379,11 @@

                 ··/**·The·privilege·to·use·a·catalog.·*/
                 ··public·static·class·UseCatalog·extends·GenericPrivilege<UseCatalog>·{
                -····private·static·final·UseCatalog·ALLOW_INSTANCE·=·new·UseCatalog(Condition.ALLOW,·Name.CREATE_CATALOG);
                -····private·static·final·UseCatalog·DENY_INSTANCE·=·new·UseCatalog(Condition.DENY,·Name.CREATE_CATALOG);
                +····private·static·final·UseCatalog·ALLOW_INSTANCE·=
                +········new·UseCatalog(Condition.ALLOW,·Name.CREATE_CATALOG);
                +····private·static·final·UseCatalog·DENY_INSTANCE·=
                +········new·UseCatalog(Condition.DENY,·Name.CREATE_CATALOG);
            ... ([410](https://github.com/apache/gravitino/actions/runs/9778148018/job/26998543927?pr=4010#step:6:412) more lines that didn't fit)
        Run './gradlew :api:spotlessApply' to fix these violations.

Please run './gradlew :api:spotlessApply' to fix these violations.

rich7420 commented 3 months ago

@xunliu thank you for your feedback.

apache / gravitino