Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0.0-alpha-2
    • Component/s: cocoon-pipeline
    • Labels:
      None

      Description

      This is a simple patch to add generics to the pipeline interface and impl. With additionally introducing marker interfaces for the component types (SAX, Stax, dom)
      this would allow compile time checks if all components have the correct type when assembling the pipeline.
      1. generics.patch
        9 kB
        Carsten Ziegeler

        Activity

        Hide
        Carsten Ziegeler added a comment -
        Patch
        Show
        Carsten Ziegeler added a comment - Patch
        Hide
        Andreas Pieber added a comment -
        I'm still the opinion that this would not work at pipeline level. The problem is that components themself decide which consumer/producer they accept (depends on the component type). The first time you come into the situation where components have to be mixed (eg. xml and picture components) the pipeline-compile-check would fail.

        Think about the following situation: A *.xhtml is inserted in the pipe with some pictures in it. According to the description in it the pictures could be transformed, saved to some place and the urls in the xhtml replaced. In this case the pipeline would look like:

        X-X-X-XP-P-P-P-PX-X-X

        Where X stands for XML-Components and P for picture-Component and XP,PX for the "Translators" (Starter and Finisher are not relevant for the example). This situation will work at the moment, but will fail with generic pipes.

        Taking the wrong components would fail the first time you create the pipeline with a SetupException so that IMO its not a real life problem at all, and more an academic problem.

        If there really have to be a compile-check it may have to be happen at component-level. Does the type of the Producer matches my Consumer (or the other way round). At least there's still the option to create a plugin for the different IDEs checking this at compile time.

        Best Regards,
        Andreas
        Show
        Andreas Pieber added a comment - I'm still the opinion that this would not work at pipeline level. The problem is that components themself decide which consumer/producer they accept (depends on the component type). The first time you come into the situation where components have to be mixed (eg. xml and picture components) the pipeline-compile-check would fail. Think about the following situation: A *.xhtml is inserted in the pipe with some pictures in it. According to the description in it the pictures could be transformed, saved to some place and the urls in the xhtml replaced. In this case the pipeline would look like: X-X-X-XP-P-P-P-PX-X-X Where X stands for XML-Components and P for picture-Component and XP,PX for the "Translators" (Starter and Finisher are not relevant for the example). This situation will work at the moment, but will fail with generic pipes. Taking the wrong components would fail the first time you create the pipeline with a SetupException so that IMO its not a real life problem at all, and more an academic problem. If there really have to be a compile-check it may have to be happen at component-level. Does the type of the Producer matches my Consumer (or the other way round). At least there's still the option to create a plugin for the different IDEs checking this at compile time. Best Regards, Andreas
        Hide
        Carsten Ziegeler added a comment -
        Why does your example from above not work with generics?
        Show
        Carsten Ziegeler added a comment - Why does your example from above not work with generics?
        Hide
        Andreas Pieber added a comment -
        Of course it would work using a general Pipeline. But it wont work using a Pipeline<StAX|SAX|Dom> (whatever). I simply wanted to point out that its possible with this generic construction to forbid completely valid constructions...
        Show
        Andreas Pieber added a comment - Of course it would work using a general Pipeline. But it wont work using a Pipeline<StAX|SAX|Dom> (whatever). I simply wanted to point out that its possible with this generic construction to forbid completely valid constructions...
        Hide
        Carsten Ziegeler added a comment - - edited
        Hmm I still don't think so :)
        As far as I understood all the discussions it is now allowed to mix sax with stax or dom components in a pipeline, they are required to use the eventing (if you regard dom as one single big event).
        Show
        Carsten Ziegeler added a comment - - edited Hmm I still don't think so :) As far as I understood all the discussions it is now allowed to mix sax with stax or dom components in a pipeline, they are required to use the eventing (if you regard dom as one single big event).
        Hide
        Steven Dolg added a comment -
        Although we cannot prevent every misconfiguration of a pipeline with this patch, I think it still makes a lot of sense.

        If a user knows which type of components (s)he will use, the pipeline can be "narrowed down" to accept only those type of components.
        If the exact type is not known or a mixture of different components is to be used, using

          Pipeline<PipelineComponent> pipeline = ...

        will yield exactly the same behavior we have now.

        IMO that is the most important aspect of this patch: we do not limit the possibilities, while still providing the option to have compile-time checks.

        The only drawback I can think of is that we will need a marker interface for every "content type" that is to be used.
        While we certainly won't need more than one (maybe two) per module, this adds another interface to the hierarchy.
        Now there were some objections that the number of interfaces already is high (or at least higher than expected), but IMO this is absolutely worth it.

        Usually I don't bother with the Pipeline module when working on a content type specific module.
        And declaring my own marker interface for my components (instead of using PipelineComponent) is actually a sound approach.
        This might also be used to clearly indicate which components are compatible - although this would rather be something to be encouraged and probably cannot be enforced, since it is only a marker interface.
        Show
        Steven Dolg added a comment - Although we cannot prevent every misconfiguration of a pipeline with this patch, I think it still makes a lot of sense. If a user knows which type of components (s)he will use, the pipeline can be "narrowed down" to accept only those type of components. If the exact type is not known or a mixture of different components is to be used, using   Pipeline<PipelineComponent> pipeline = ... will yield exactly the same behavior we have now. IMO that is the most important aspect of this patch: we do not limit the possibilities, while still providing the option to have compile-time checks. The only drawback I can think of is that we will need a marker interface for every "content type" that is to be used. While we certainly won't need more than one (maybe two) per module, this adds another interface to the hierarchy. Now there were some objections that the number of interfaces already is high (or at least higher than expected), but IMO this is absolutely worth it. Usually I don't bother with the Pipeline module when working on a content type specific module. And declaring my own marker interface for my components (instead of using PipelineComponent) is actually a sound approach. This might also be used to clearly indicate which components are compatible - although this would rather be something to be encouraged and probably cannot be enforced, since it is only a marker interface.
        Hide
        Carsten Ziegeler added a comment -
        Yes, even with this patch it's still possible to mix different component types and yes, we have to introduce new marker interfaces :) (and yes it was me how
        mentioned that there are too many interfaces already...), but if you're just interested in one component type, e.g. sax, then it's just one additional interface.

        So if noone objects I'll apply the patch in the next days.
        Show
        Carsten Ziegeler added a comment - Yes, even with this patch it's still possible to mix different component types and yes, we have to introduce new marker interfaces :) (and yes it was me how mentioned that there are too many interfaces already...), but if you're just interested in one component type, e.g. sax, then it's just one additional interface. So if noone objects I'll apply the patch in the next days.
        Hide
        Grzegorz Kossakowski added a comment -
        > As far as I understood all the discussions it is now allowed to mix sax with stax or dom components in a pipeline,
        > they are required to use the eventing (if you regard dom as one single big event).

        Is this event-based communication guaranteed at PipelineComponent level or it's left to implementation how to design it?

        > If a user knows which type of components (s)he will use, the pipeline can be "narrowed down" to accept only those type of components.
        > If the exact type is not known or a mixture of different components is to be used, using

        How it's possible that user is using a component and does not know what kind of input/output this component deals with?

        I agree with Steven's point on the need for creation of marker interfaces. If they are going to be purely marker interfaces then I'm quite concerned about this solution.

        Is it only my omission or this patch does not solve a problem with Pipeline results outlined by Reinhard: http://article.gmane.org/gmane.text.xml.cocoon.devel/79362

        To sum up my opinion I'm -0 on this patch. In itself it's not bad (and not invading) but I think this patch tries to tweak broken foundations. I would prefer if we could agree on Pipelines and PipelineComponent fundamental design decisions and goals before we move to providing patches.

        There were two different approaches presented in form of patches that left on or another side unhappy so such an agreement seems to be the best solution. What about a serious discussion on this topic during ApacheCon EU this year? I don't want to suspend the whole discussion for two months, of course.
        Show
        Grzegorz Kossakowski added a comment - > As far as I understood all the discussions it is now allowed to mix sax with stax or dom components in a pipeline, > they are required to use the eventing (if you regard dom as one single big event). Is this event-based communication guaranteed at PipelineComponent level or it's left to implementation how to design it? > If a user knows which type of components (s)he will use, the pipeline can be "narrowed down" to accept only those type of components. > If the exact type is not known or a mixture of different components is to be used, using How it's possible that user is using a component and does not know what kind of input/output this component deals with? I agree with Steven's point on the need for creation of marker interfaces. If they are going to be purely marker interfaces then I'm quite concerned about this solution. Is it only my omission or this patch does not solve a problem with Pipeline results outlined by Reinhard: http://article.gmane.org/gmane.text.xml.cocoon.devel/79362 To sum up my opinion I'm -0 on this patch. In itself it's not bad (and not invading) but I think this patch tries to tweak broken foundations. I would prefer if we could agree on Pipelines and PipelineComponent fundamental design decisions and goals before we move to providing patches. There were two different approaches presented in form of patches that left on or another side unhappy so such an agreement seems to be the best solution. What about a serious discussion on this topic during ApacheCon EU this year? I don't want to suspend the whole discussion for two months, of course.
        Hide
        Steven Dolg added a comment -
        > Is this event-based communication guaranteed at PipelineComponent level or it's left to implementation how to design it?

        The supplied patch does not change how the components interact with each other.
        The PipelineComponent interface remains unchanged.


        > How it's possible that user is using a component and does not know what kind of input/output this component deals with?

        This is always the case when the Pipeline is assembled at runtime from a provided description, like the Sitemap.
        While the PipelineAPI is intended do be used with direct calls from the user's code, we should not forget that automatic pipeline construction using some kind of description is still a valid use case and that basically no assumptions about the actual components can be made in such a case (iow Generics are pretty much worthless for that)
        Show
        Steven Dolg added a comment - > Is this event-based communication guaranteed at PipelineComponent level or it's left to implementation how to design it? The supplied patch does not change how the components interact with each other. The PipelineComponent interface remains unchanged. > How it's possible that user is using a component and does not know what kind of input/output this component deals with? This is always the case when the Pipeline is assembled at runtime from a provided description, like the Sitemap. While the PipelineAPI is intended do be used with direct calls from the user's code, we should not forget that automatic pipeline construction using some kind of description is still a valid use case and that basically no assumptions about the actual components can be made in such a case (iow Generics are pretty much worthless for that)
        Hide
        Reinhard Poetz added a comment -
        I think this patch will be the best solution that the Java programming language can support. Grek, your ideas are interesting but I fail to see how this could ever work with Java.

        So here's my +1 for Carsten's patch.

        @Grek: We won't reach beta before Amsterdam, which means that we can still change core contracts.
        Show
        Reinhard Poetz added a comment - I think this patch will be the best solution that the Java programming language can support. Grek, your ideas are interesting but I fail to see how this could ever work with Java. So here's my +1 for Carsten's patch. @Grek: We won't reach beta before Amsterdam, which means that we can still change core contracts.
        Hide
        Grzegorz Kossakowski added a comment - - edited
        > The supplied patch does not change how the components interact with each other.
        > The PipelineComponent interface remains unchanged.

        I didn't formulate my question clearly: what is our intention. Do we want a generic API for event exchange or each pair of components will be dealing with this in it's implementation by doing some of instanceof checks?

        >This is always the case when the Pipeline is assembled at runtime from a provided description, like the Sitemap.
        > While the PipelineAPI is intended do be used with direct calls from the user's code, we should not forget that automatic pipeline
        > construction using some kind of description is still a valid use case and that basically no assumptions about
        > the actual components can be made in such a case (iow Generics are pretty much worthless for that)

        It's not entirely true that generics are worhtless at runtime. Take a look at quick prototype:
        ---------------------------------------- PipelineComponent.java ----------------------------------------
        package test;

        public interface PipelineComponent<T, U> {
        U execute(T event);
        }

        ---------------------------------------- TestComponent.java ----------------------------------------
        package test;

        import java.util.LinkedList;
        import java.util.List;

        public class TestCompoent implements PipelineComponent<String, List<String>> {

        public List<String> execute(String event) {
        List<String> list = new LinkedList<String>();
        list.add(event);
        return list;
        }

        }

        ---------------------------------------- main.java ----------------------------------------
        package test;

        import java.lang.reflect.ParameterizedType;
        import java.lang.reflect.Type;

        public class main {

        public static void main(String[] args) {
        Type[] interfaces = TestCompoent.class.getGenericInterfaces();
        for (Type i : interfaces) {
        if (i instanceof ParameterizedType) {
        ParameterizedType pt = (ParameterizedType)i;
        if (pt.getRawType().equals(PipelineComponent.class)) {
        Type[] typeArguments = pt.getActualTypeArguments();
        Type input = typeArguments[0];
        Type output = typeArguments[1];
        System.out.println(TestCompoent.class + " is " +
        " PipelineComponent<" + input + ", " + output + ">");
        }
        }
        }
        }

        }


        This gives me following output:
        class test.TestCompoent is PipelineComponent<class java.lang.String, java.util.List<java.lang.String>>

        My point is that Pipeline *can* and *should* check if it's valid (given components can be combined). Of course this will work only if one component accepts one type of input and produces one type of output but I don't see this as a problem.
        Show
        Grzegorz Kossakowski added a comment - - edited > The supplied patch does not change how the components interact with each other. > The PipelineComponent interface remains unchanged. I didn't formulate my question clearly: what is our intention. Do we want a generic API for event exchange or each pair of components will be dealing with this in it's implementation by doing some of instanceof checks? >This is always the case when the Pipeline is assembled at runtime from a provided description, like the Sitemap. > While the PipelineAPI is intended do be used with direct calls from the user's code, we should not forget that automatic pipeline > construction using some kind of description is still a valid use case and that basically no assumptions about > the actual components can be made in such a case (iow Generics are pretty much worthless for that) It's not entirely true that generics are worhtless at runtime. Take a look at quick prototype: ---------------------------------------- PipelineComponent.java ---------------------------------------- package test; public interface PipelineComponent<T, U> { U execute(T event); } ---------------------------------------- TestComponent.java ---------------------------------------- package test; import java.util.LinkedList; import java.util.List; public class TestCompoent implements PipelineComponent<String, List<String>> { public List<String> execute(String event) { List<String> list = new LinkedList<String>(); list.add(event); return list; } } ---------------------------------------- main.java ---------------------------------------- package test; import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; public class main { public static void main(String[] args) { Type[] interfaces = TestCompoent.class.getGenericInterfaces(); for (Type i : interfaces) { if (i instanceof ParameterizedType) { ParameterizedType pt = (ParameterizedType)i; if (pt.getRawType().equals(PipelineComponent.class)) { Type[] typeArguments = pt.getActualTypeArguments(); Type input = typeArguments[0]; Type output = typeArguments[1]; System.out.println(TestCompoent.class + " is " + " PipelineComponent<" + input + ", " + output + ">"); } } } } } This gives me following output: class test.TestCompoent is PipelineComponent<class java.lang.String, java.util.List<java.lang.String>> My point is that Pipeline *can* and *should* check if it's valid (given components can be combined). Of course this will work only if one component accepts one type of input and produces one type of output but I don't see this as a problem.
        Hide
        Grzegorz Kossakowski added a comment -
        Looks like our JIRA set-up sucks and JIRA does not follow comment formatting.

        I'm moving with discussion to mailing list.
        Show
        Grzegorz Kossakowski added a comment - Looks like our JIRA set-up sucks and JIRA does not follow comment formatting. I'm moving with discussion to mailing list.
        Hide
        Steven Dolg added a comment -
        > This gives me following output:
        > class test.TestCompoent is PipelineComponent<class java.lang.String, java.util.List<java.lang.String>>

        Of course you can do this and use the generic parameters to check for compatibility.
        But I don't see how this is any difference than the instanceof check, we have now.
        IMO an "component instanceof StAXConsumer" is just as good as an "firstComponent.getOutputType() == lastComponent.getInputType()".
        Of course the latter one looks much more sophisticated but basically you're just checking for some specific class.

        And I also don't see what's the advantage in having one class performing the compatibility check (and supporting all the little exceptions from the common case which might be necessary over time) instead of letting the components do it themselves.


        I'm still wondering what the input and output type of the SAX components would be and how the callbacks should be integrated/defined in the PipelineComponent interface.
        Maybe we can have an example, so that this can become less speculative.
        Show
        Steven Dolg added a comment - > This gives me following output: > class test.TestCompoent is PipelineComponent<class java.lang.String, java.util.List<java.lang.String>> Of course you can do this and use the generic parameters to check for compatibility. But I don't see how this is any difference than the instanceof check, we have now. IMO an "component instanceof StAXConsumer" is just as good as an "firstComponent.getOutputType() == lastComponent.getInputType()". Of course the latter one looks much more sophisticated but basically you're just checking for some specific class. And I also don't see what's the advantage in having one class performing the compatibility check (and supporting all the little exceptions from the common case which might be necessary over time) instead of letting the components do it themselves. I'm still wondering what the input and output type of the SAX components would be and how the callbacks should be integrated/defined in the PipelineComponent interface. Maybe we can have an example, so that this can become less speculative.
        Hide
        Reinhard Poetz added a comment -
        Patch was already applied.
        Show
        Reinhard Poetz added a comment - Patch was already applied.

          People

          • Assignee:
            Unassigned
            Reporter:
            Carsten Ziegeler
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development