Tapestry 5
  1. Tapestry 5
  2. TAP5-165

Components which use PrimaryKeyEncoder should be changed to use ValueEncoder, and PrimaryKeyEncoder should be deprecated

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 5.0.16
    • Fix Version/s: 5.1.0.0
    • Component/s: None
    • Labels:
      None
    • Environment:
      any

      Description

      While working on an application, I noticed that my objects were being serialized "weird" into a form by the loop component. I realized that I hadn't provided the primary key encoder, and once I did things worked as expected. That got me to thinking that it would be nice if the Loop component, and other components that rely on PrimaryKeyEncoders, could check to see if there is an encoder available for the value-type, if none is explicitly bound by the user. That way, module-authors could provide PrimaryKeyEncoders that makes things work "like magic".
      For example, tapestry-hibernate could contribute PrimaryKeyEncoders for each entity type so that the objects are automatically, and properly, encoded into forms.

        Activity

        Hide
        Robert Zeigler added a comment -

        After thinking about this some more, it occurred to me that the right solution is probably to nix PrimaryKeyEncoder entirely.
        Instead, Loop and grid, like many other components, should be using ValueEncoders to do the job. Consider the implementation of the hibernate value encoders: they are effectively PrimaryKeyEncoders. The main difference between the two interfaces is that PrimaryKeyEncoder is slightly more lenient: the "client" value merely has to be serializable, whereas the ValueEncoder must encode to a string. But the key is ultimately going to be stored on the client as a string, so there seems to be no reason why you wouldn't just use ValueEncoder. This would get rid of one redundant interface (PrimaryKeyEncoder) /and/ allow Loop, Grid, etc. to tie into the very nice set of services available for looking up ValueEncoders, so that you can contribute your value encoder(s), and have stuff "just work".

        Show
        Robert Zeigler added a comment - After thinking about this some more, it occurred to me that the right solution is probably to nix PrimaryKeyEncoder entirely. Instead, Loop and grid, like many other components, should be using ValueEncoders to do the job. Consider the implementation of the hibernate value encoders: they are effectively PrimaryKeyEncoders. The main difference between the two interfaces is that PrimaryKeyEncoder is slightly more lenient: the "client" value merely has to be serializable, whereas the ValueEncoder must encode to a string. But the key is ultimately going to be stored on the client as a string, so there seems to be no reason why you wouldn't just use ValueEncoder. This would get rid of one redundant interface (PrimaryKeyEncoder) /and/ allow Loop, Grid, etc. to tie into the very nice set of services available for looking up ValueEncoders, so that you can contribute your value encoder(s), and have stuff "just work".
        Hide
        Howard M. Lewis Ship added a comment -

        Yes but, the point of PrimaryKeyEncoder is two fold:

        1) The primary key will be serialized, so the encoded value doesn't have to be a string (ValueEncoder is full encoding objects into URLs or DOM attributes).
        2) PKE includes the ability to ask for a pre-load of multiple keys, which is very important scalability-wise. One database query, not N.

        Show
        Howard M. Lewis Ship added a comment - Yes but, the point of PrimaryKeyEncoder is two fold: 1) The primary key will be serialized, so the encoded value doesn't have to be a string (ValueEncoder is full encoding objects into URLs or DOM attributes). 2) PKE includes the ability to ask for a pre-load of multiple keys, which is very important scalability-wise. One database query, not N.
        Hide
        Robert Zeigler added a comment -

        shrug
        Pre-loading from PKE is nice... I don't see, however, why it couldn't be incorporated directly into ValueEncoder.

        My real gripe is that there are simply too many "type conversion" systems in T5.
        We have:
        Translators
        ValueEncoders
        PrimaryKeyEncoders
        TypeCoercers

        That's four different systems. Granted, each does something /slightly/ different:
        Translator -> handles text input /by the user/, to be converted to some non-text type.
        ValueEncoders -> handle storing a string representation of an object into a url for "pretty" urls
        PrimaryKeyEncoders -> specifically store the primary key of an object into a form, and retrieve the value again, allowing for preloading of multiple keys.
        TypeCoercers -> convert to and from parameter types within the framework.

        But there's so much overlap between them... I like t5... but this, to me, is a glaring wart of over-complexity.

        In any event, if the 4 systems are all here to stay, then my original description of the issue (allowing for configurable pk encoders, just like value encoders, so module developers can make them work "like magic" as we do with value encoders) is still valid.

        Show
        Robert Zeigler added a comment - shrug Pre-loading from PKE is nice... I don't see, however, why it couldn't be incorporated directly into ValueEncoder. My real gripe is that there are simply too many "type conversion" systems in T5. We have: Translators ValueEncoders PrimaryKeyEncoders TypeCoercers That's four different systems. Granted, each does something /slightly/ different: Translator -> handles text input /by the user/, to be converted to some non-text type. ValueEncoders -> handle storing a string representation of an object into a url for "pretty" urls PrimaryKeyEncoders -> specifically store the primary key of an object into a form, and retrieve the value again, allowing for preloading of multiple keys. TypeCoercers -> convert to and from parameter types within the framework. But there's so much overlap between them... I like t5... but this, to me, is a glaring wart of over-complexity. In any event, if the 4 systems are all here to stay, then my original description of the issue (allowing for configurable pk encoders, just like value encoders, so module developers can make them work "like magic" as we do with value encoders) is still valid.
        Hide
        Howard M. Lewis Ship added a comment -

        But in many cases they are "layered". Most of the time a ValueEncoder is just a wrapper around TypeCoercer. Likewise, I think we can automatically generate PKEs from ValueEncoders and do it invisibly.

        Show
        Howard M. Lewis Ship added a comment - But in many cases they are "layered". Most of the time a ValueEncoder is just a wrapper around TypeCoercer. Likewise, I think we can automatically generate PKEs from ValueEncoders and do it invisibly.
        Hide
        Kevin Menard added a comment -

        I had voiced similar concerns in the past. It can just be downright confusing. Just earlier today I was trying to help someone (that's been using the framework for a while) on IRC understand the differences, which are quite subtle.

        Show
        Kevin Menard added a comment - I had voiced similar concerns in the past. It can just be downright confusing. Just earlier today I was trying to help someone (that's been using the framework for a while) on IRC understand the differences, which are quite subtle.
        Hide
        Robert Zeigler added a comment -

        The evidence of the overcomplexity is that the framework, itself, trips up on it, especially in regards to layering.
        Take the following rather trivial example:

        public class SomeComponent {
        @Parameter
        private Object[] parameters;

        @Component(parameters="context=inherit:context")
        private ActionLink someAction;

        @Inject
        private ComponentResources resources;

        void onActionFromSomeAction(Object[] parameters)

        { resources.triggerEvent("SomeComponentEvent",parameters,null); }

        }

        Is there a problem with that code? Try writing an event handler for SomeComponentEvent which takes a hibernate entity, and then try passing in a hibernate entity to SomeComponent.

        You'll wind up with an exception: no coercion found from string to type xxx (where xxx is the type of your hibernate entity).

        What happens is that someAction uses ValueEncoder to encode the hibernate entity to string.
        But because onAction takes the parameters as an object array, they aren't decoded.
        Then the event handling code will find the SomeComponentEventHandler, and try to convert the parameters to match the method signature of the handler.
        But the event handling code uses TypeCoercer directly, rather than ValueEncoders.
        So you've got a parameter that is encoded with ValueEncoder, but decoded with TypeCoercer.
        And this is what I mean about overly complex.
        I can file a separate jira for the specific instance described above, but the bigger issue is having 4 type conversion systems in the first place.
        This isn't the first place where this same issue (value encoded by one type coercion system are decoded by another one elsewhere) has come up, because I reported exactly this issue with the form component back in Feb. in TAPESTRY-2177.

        If T5 is going to be competitive against frameworks built in dynamic languages, it absolutely has to handle type coercion so seamlessly that the user /never/ has to think about it. Right now, there's way too much thinking involved, starting with having 4 ways of converting types.

        Show
        Robert Zeigler added a comment - The evidence of the overcomplexity is that the framework, itself, trips up on it, especially in regards to layering. Take the following rather trivial example: public class SomeComponent { @Parameter private Object[] parameters; @Component(parameters="context=inherit:context") private ActionLink someAction; @Inject private ComponentResources resources; void onActionFromSomeAction(Object[] parameters) { resources.triggerEvent("SomeComponentEvent",parameters,null); } } Is there a problem with that code? Try writing an event handler for SomeComponentEvent which takes a hibernate entity, and then try passing in a hibernate entity to SomeComponent. You'll wind up with an exception: no coercion found from string to type xxx (where xxx is the type of your hibernate entity). What happens is that someAction uses ValueEncoder to encode the hibernate entity to string. But because onAction takes the parameters as an object array, they aren't decoded. Then the event handling code will find the SomeComponentEventHandler, and try to convert the parameters to match the method signature of the handler. But the event handling code uses TypeCoercer directly, rather than ValueEncoders. So you've got a parameter that is encoded with ValueEncoder, but decoded with TypeCoercer. And this is what I mean about overly complex. I can file a separate jira for the specific instance described above, but the bigger issue is having 4 type conversion systems in the first place. This isn't the first place where this same issue (value encoded by one type coercion system are decoded by another one elsewhere) has come up, because I reported exactly this issue with the form component back in Feb. in TAPESTRY-2177 . If T5 is going to be competitive against frameworks built in dynamic languages, it absolutely has to handle type coercion so seamlessly that the user /never/ has to think about it. Right now, there's way too much thinking involved, starting with having 4 ways of converting types.
        Hide
        Howard M. Lewis Ship added a comment -

        About your last example: use the following:

        void onActionFromSomeAction(EventContext context)

        { resources.triggerContextEvent("SomeComponentEvent",context,null); }

        Show
        Howard M. Lewis Ship added a comment - About your last example: use the following: void onActionFromSomeAction(EventContext context) { resources.triggerContextEvent("SomeComponentEvent",context,null); }
        Hide
        Howard M. Lewis Ship added a comment -

        The PrimarykeyEncoder could be deprecated and eliminated. The Loop component could fire an event to inform its container that it should preload a set of keys.

        But I still don't like it, because it forces on the application the work of changing the String that the Loop component knows about back into an id (say, a Long) that can be used to pre-load.

        Here's an idea ... if we ditch the pre-loading concept entirely (it can be rolled into its own Hibernate-specific component) .. we can simplify Loop AND address this issue.

        I would suggest that we keep the encoder parameter of Loop (for at least one release) and emit a warning if it is used. We should then add new parameter (of type ValueEncoder).

        Alternately, perhaps we can provide a TypeCoercion from PKE to ValueEncoder; then we can have compatibility. Under the new system, a default ValueEncoder could be derived from the value parameters' type.

        The only disadvantage of this is that a String version of an entity id is bulkier than a wrapped primitive (a Long). However, GZIP compression should take care of that reasonably.

        Show
        Howard M. Lewis Ship added a comment - The PrimarykeyEncoder could be deprecated and eliminated. The Loop component could fire an event to inform its container that it should preload a set of keys. But I still don't like it, because it forces on the application the work of changing the String that the Loop component knows about back into an id (say, a Long) that can be used to pre-load. Here's an idea ... if we ditch the pre-loading concept entirely (it can be rolled into its own Hibernate-specific component) .. we can simplify Loop AND address this issue. I would suggest that we keep the encoder parameter of Loop (for at least one release) and emit a warning if it is used. We should then add new parameter (of type ValueEncoder). Alternately, perhaps we can provide a TypeCoercion from PKE to ValueEncoder; then we can have compatibility. Under the new system, a default ValueEncoder could be derived from the value parameters' type. The only disadvantage of this is that a String version of an entity id is bulkier than a wrapped primitive (a Long). However, GZIP compression should take care of that reasonably.
        Hide
        Robert Zeigler added a comment -

        The irony of this issue is that Loop originally used ValueEncoder. Now that 5.0 has hit "final" (5.0.18), I'm not sure changing this is the right thing to do. For better or worse, we've introduced the PKE interface and parameter to the masses. So perhaps PKE=>ValueEncoder coercion is the best bet. Alternatively, my original summary may still hold: if there was a PrimaryKeyEncoderSource, just like ValueEncoderSource, then module and application authors could still make the appropriate contributions and not have to worry about specifying the PKE when using Loop. The two strategies could be combined: Loop uses PKESource to lookup the PKE; the PKE has a tapestry-contributed fallback mapping that checks for ValueEncoders for the specified type. The tricky thing is determining the type from the source collection, but the same heuristics used by Grid can be applied to Loop.

        Show
        Robert Zeigler added a comment - The irony of this issue is that Loop originally used ValueEncoder. Now that 5.0 has hit "final" (5.0.18), I'm not sure changing this is the right thing to do. For better or worse, we've introduced the PKE interface and parameter to the masses. So perhaps PKE=>ValueEncoder coercion is the best bet. Alternatively, my original summary may still hold: if there was a PrimaryKeyEncoderSource, just like ValueEncoderSource, then module and application authors could still make the appropriate contributions and not have to worry about specifying the PKE when using Loop. The two strategies could be combined: Loop uses PKESource to lookup the PKE; the PKE has a tapestry-contributed fallback mapping that checks for ValueEncoders for the specified type. The tricky thing is determining the type from the source collection, but the same heuristics used by Grid can be applied to Loop.
        Hide
        Howard M. Lewis Ship added a comment -

        This is coming along well, but I'm having trouble with the automatic coercion from PrimaryKeyEncoder to ValueEncoder. The problem is using reflection to dig out the key type, which is very necessary.

        Show
        Howard M. Lewis Ship added a comment - This is coming along well, but I'm having trouble with the automatic coercion from PrimaryKeyEncoder to ValueEncoder. The problem is using reflection to dig out the key type, which is very necessary.
        Hide
        Howard M. Lewis Ship added a comment -

        The problem is the way generics are implemented. The key type is lost to type erasure. I wish PKE had one more method on it: key type.

        I may have to add it.

        Show
        Howard M. Lewis Ship added a comment - The problem is the way generics are implemented. The key type is lost to type erasure. I wish PKE had one more method on it: key type. I may have to add it.

          People

          • Assignee:
            Howard M. Lewis Ship
            Reporter:
            Robert Zeigler
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development