sizovs / PipelinR

PipelinR is a lightweight command processing pipeline ❍ ⇢ ❍ ⇢ ❍ for your Java awesome app.
https://github.com/sizovs/PipelinR
MIT License
420 stars 59 forks source link

Can't override method matches in Notification.Handler #24

Closed vitalii-honchar closed 2 years ago

vitalii-honchar commented 2 years ago

Problem Can't override matches method in implementation of Notification.Handler.

Steps to Reproduce Just launch this test:

import org.junit.jupiter.api.Test;

public class PipelinrMatchesTest {

    class Event1 implements Notification {}

    class Event2 implements Notification {}

    class TestHandler1 implements Notification.Handler<Event1> {

        @Override
        public void handle(Event1 notification) {

        }

        @Override
        public boolean matches(Event1 notification) {
            return Notification.Handler.super.matches(notification);
        }
    }

    class TestHandler2 implements Notification.Handler<Event2> {
        @Override
        public void handle(Event2 notification) {

        }
    }

    @Test
    void testMatches() {
        Notification.Handler handler1 = new TestHandler1();
        Notification.Handler handler2 = new TestHandler2();

        Event1 event1 = new Event1();
        Event2 event2 = new Event2();

        System.out.println(handler1.matches(event1));
        System.out.println(handler2.matches(event2));

        System.out.println(handler1.matches(event2));
        System.out.println(handler2.matches(event1));
    }
}

Exception will throws on line System.out.println(handler1.matches(event2));.

Class an.awesome.pipelinr.Pipelinr uses handlers without generic specification, so this will be reproduced in Pipelinr usage with override method matches.

Expected Result Success complete test execution.

Actual Result

class an.awesome.pipelinr.PipelinrMatchesTest$Event2 cannot be cast to class an.awesome.pipelinr.PipelinrMatchesTest$Event1 (an.awesome.pipelinr.PipelinrMatchesTest$Event2 and an.awesome.pipelinr.PipelinrMatchesTest$Event1 are in unnamed module of loader 'app')
java.lang.ClassCastException: class an.awesome.pipelinr.PipelinrMatchesTest$Event2 cannot be cast to class an.awesome.pipelinr.PipelinrMatchesTest$Event1 (an.awesome.pipelinr.PipelinrMatchesTest$Event2 and an.awesome.pipelinr.PipelinrMatchesTest$Event1 are in unnamed module of loader 'app')
    at an.awesome.pipelinr.PipelinrMatchesTest$TestHandler1.matches(PipelinrMatchesTest.java:13)
    at an.awesome.pipelinr.PipelinrMatchesTest.testMatches(PipelinrMatchesTest.java:49)
sizovs commented 2 years ago

Hey

A veeery quick look at the code tells me that's it's expected behavior, as handlers can only handle events that match its generic signature:

👌 System.out.println(handler1.matches(event1)); 👌 System.out.println(handler2.matches(event2));

❌ System.out.println(handler1.matches(event2)); ❌ System.out.println(handler2.matches(event1));

Did I miss something? What would be the real use case for that?

vitalii-honchar commented 2 years ago

Hello, Code example:

import java.util.stream.Stream;
import org.junit.jupiter.api.Test;

public class OverrideMatchesTest {

    class Event1 implements Notification {}

    class Event2 implements Notification {}

    class TestHandler1 implements Notification.Handler<Event1> {

        @Override
        public void handle(Event1 notification) {
            System.out.println("Event1: " + notification);
        }

        @Override
        public boolean matches(Event1 notification) {
            return Notification.Handler.super.matches(notification);
        }
    }

    class TestHandler2 implements Notification.Handler<Event2> {
        @Override
        public void handle(Event2 notification) {
            System.out.println("Event 2: " + notification);
        }
    }

    @Test
    void testMatches() {
        TestHandler1 handler1 = new TestHandler1();
        TestHandler2 handler2 = new TestHandler2();

        Pipelinr pipelinr = new Pipelinr()
                .with(() -> Stream.of(handler1, handler2));

        new Event2().send(pipelinr);
    }
}

Expected result is print "Event 2: " + notification, but actual result is class cast exception:

class an.awesome.pipelinr.OverrideMatchesTest$Event2 cannot be cast to class an.awesome.pipelinr.OverrideMatchesTest$Event1 (an.awesome.pipelinr.OverrideMatchesTest$Event2 and an.awesome.pipelinr.OverrideMatchesTest$Event1 are in unnamed module of loader 'app')
java.lang.ClassCastException: class an.awesome.pipelinr.OverrideMatchesTest$Event2 cannot be cast to class an.awesome.pipelinr.OverrideMatchesTest$Event1 (an.awesome.pipelinr.OverrideMatchesTest$Event2 and an.awesome.pipelinr.OverrideMatchesTest$Event1 are in unnamed module of loader 'app')

Use case for this:

  1. I have two handlers for one type
  2. One handler must override method matches for execute my logic for match event
  3. Second handler must receive all events of specified type, thus use default method matches
sizovs commented 2 years ago

Got it. Do you have time/energy/capacity for a PR?

vitalii-honchar commented 2 years ago

Sorry, I'm not sure that I will have a time for fix it right now.

I have some workaround for now:

  interface GenericHandler<N extends Notification> extends Notification.Handler<Notification> {

      @SuppressWarnings("unchecked")
      @Override
      default void handle(Notification notification) {
          if (matches(notification)) {
              handleNotification((N) notification);
          }
      }

      void handleNotification(N notification);

      @Override
      boolean matches(Notification notification);
  }
sizovs commented 2 years ago

Hi @vitalii-honchar

I've added a test demonstrating custom notification handling: https://github.com/sizovs/PipelinR/commit/6d1f9e82a8015e47accc4de58ef36d7420c83d23.

1) To have a "catch-all" notification handler, don't override matches. 2) Custom notification handlers work as long as you don't call super.matches() – which, I suppose, you can completely avoid calling in your usecase. If you can't, instead of delegating to super, implement your own comparison.

Something like that:

class TestHandler1 implements Notification.Handler<Event1> {

    @Override
    public void handle(Event1 notification) {
        System.out.println("Event1: " + notification);
    }

    @Override
    public boolean matches(Event1 notification) {
        return notification.getClass().equals(Event1.class);
    }
}