Hi all, I was adding some tests for Query as I just found many basic operations are uncovered, and found an annoying issue. When using cache.putAll(values); the Query interceptor will iterate on each entry, transform each entry key and replace those entry keys in string form with the new indexed values. The problem is *which index* : the index mapping information is bound to the indexed type. So the normal put/replace/remove operations actually rely on the return value to know where to direct the proper index operation (to which index) - if any.

With putAll() returning void, I don't have the return values. I could hack this to "unroll" in several individual put() operations but I don't think that should happen in an interceptor. Also this would defeat the purpose of using putAll, killing it's efficiency over using several put operations.

I would generally expect people to put objects of the same type when replacing a key.. so this works fine usually looking at the runtime type of the new value (as it's doing), but it's not correct generally. Worse, I can't detect the mismatch and log some warning.

Any suggestion? I'm tempted to say I need to retrieve all previous values. I actually need only the types :-/ A simple solution would be to disallow using multiple indexes for the same cache, but that's not a welcome change. I'd prefer to throw an exception on putAll : not supported when indexing is enabled.

On 27 March 2012 15:37, Manik Surtani <manik(a)jboss.org&gt; wrote: > > On 27 Mar 2012, at 14:51, Sanne Grinovero wrote: > >> Hi all, >> I was adding some tests for Query as I just found many basic >> operations are uncovered, and found an annoying issue. >> >> When using >> >> cache.putAll(values); >> >> the Query interceptor will iterate on each entry, transform each entry >> key and replace those entry keys in string form with the new indexed >> values. >> >> The problem is *which index* : the index mapping information is bound >> to the indexed type. So the normal put/replace/remove operations >> actually rely on the return value to know where to direct the proper >> index operation (to which index) - if any. > > So even operations where return values are suppressed won't work? Depends, most cases will work anyway, but definitely some cases could misbehave when suppressing return values. I guess my next answers explain this better.. >> With putAll() returning void, I don't have the return values. >> >> I could hack this to "unroll" in several individual put() operations >> but I don't think that should happen in an interceptor. Also this >> would defeat the purpose of using putAll, killing it's efficiency over >> using several put operations. > > No, that's no good… > >> >> I would generally expect people to put objects of the same type when >> replacing a key.. so this works fine usually looking at the runtime >> type of the new value (as it's doing), but it's not correct generally. >> Worse, I can't detect the mismatch and log some warning. > > Why is that not correct? IMO, working on the type of the new value is more correct than doing so on the old value … Yes I definitely need the type of the new value to index the one one; but I also need the type of the previous value if it existed, to know how to cleanup existing index entries. >> Any suggestion? I'm tempted to say I need to retrieve all previous >> values. I actually need only the types :-/ >> >> A simple solution would be to disallow using multiple indexes for the >> same cache, but that's not a welcome change. I'd prefer to throw an >> exception on putAll : not supported when indexing is enabled. > > You could also maintain a type map? Keys -> Type. But that's unnecessarily voluminous. What's wrong with working off the type of the new value? Are you concerned about the types changing and hence not being able to clean up stale indexes? Exactly: the problem is with cleanup of previous index entries. Granted I don't expect this to be common; personally I would favour an application design in which - for a specific key - the type of the value could be determined by the key. But if you expect to support this: put( "name one", new SomeStuff("value") ); put( "name one", new SomeDifferentStuff("value") ); then, iff SomeStuff and SomeDifferentStuff are using different index names A and B, the second put needs to know the type of the previous value to remote "name one" from the index A while adding the new value to index B. For this to work I'm "having a look" at the type of the return value of the put. So indeed if a) you're suppressing return values b) two different types are indexed on different indexes c) you're reusing the same key instance for a different type then you get in trouble. Of course that's not common, and there are many workarounds but tricky to explain; worst of all, I can't detect the problem and warn. Cheers, Sanne > Cheers > Manik > > -- > Manik Surtani > manik(a)jboss.org > twitter.com/maniksurtani > > Lead, Infinispan > http://www.infinispan.org > > > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

I think it is fair to say that it is user error if the user changes the type of value of a key to one in a different index? You're right, detecting can be hard … On 27 Mar 2012, at 15:52, Sanne Grinovero wrote: > On 27 March 2012 15:37, Manik Surtani <manik(a)jboss.org&gt; wrote: >> >> On 27 Mar 2012, at 14:51, Sanne Grinovero wrote: >> >>> Hi all, >>> I was adding some tests for Query as I just found many basic >>> operations are uncovered, and found an annoying issue. >>> >>> When using >>> >>> cache.putAll(values); >>> >>> the Query interceptor will iterate on each entry, transform each entry >>> key and replace those entry keys in string form with the new indexed >>> values. >>> >>> The problem is *which index* : the index mapping information is bound >>> to the indexed type. So the normal put/replace/remove operations >>> actually rely on the return value to know where to direct the proper >>> index operation (to which index) - if any. >> >> So even operations where return values are suppressed won't work? > > Depends, most cases will work anyway, but definitely some cases could > misbehave when suppressing return values. > I guess my next answers explain this better.. > > >>> With putAll() returning void, I don't have the return values. >>> >>> I could hack this to "unroll" in several individual put() operations >>> but I don't think that should happen in an interceptor. Also this >>> would defeat the purpose of using putAll, killing it's efficiency over >>> using several put operations. >> >> No, that's no good… >> >>> >>> I would generally expect people to put objects of the same type when >>> replacing a key.. so this works fine usually looking at the runtime >>> type of the new value (as it's doing), but it's not correct generally. >>> Worse, I can't detect the mismatch and log some warning. >> >> Why is that not correct? IMO, working on the type of the new value is more correct than doing so on the old value … > > Yes I definitely need the type of the new value to index the one one; > but I also need the type of the previous value if it existed, to know > how to cleanup existing index entries. > > >>> Any suggestion? I'm tempted to say I need to retrieve all previous >>> values. I actually need only the types :-/ >>> >>> A simple solution would be to disallow using multiple indexes for the >>> same cache, but that's not a welcome change. I'd prefer to throw an >>> exception on putAll : not supported when indexing is enabled. >> >> You could also maintain a type map? Keys -> Type. But that's unnecessarily voluminous. What's wrong with working off the type of the new value? Are you concerned about the types changing and hence not being able to clean up stale indexes? > > Exactly: the problem is with cleanup of previous index entries. > Granted I don't expect this to be common; personally I would favour an > application design in which - for a specific key - the type of the > value could be determined by the key. > But if you expect to support this: > > put( "name one", new SomeStuff("value") ); > put( "name one", new SomeDifferentStuff("value") ); > > then, iff SomeStuff and SomeDifferentStuff are using different index > names A and B, the second put needs to know the type of the previous > value to remote "name one" from the index A while adding the new value > to index B. > > For this to work I'm "having a look" at the type of the return value of the put. > > So indeed if > a) you're suppressing return values > b) two different types are indexed on different indexes > c) you're reusing the same key instance for a different type > > then you get in trouble. Of course that's not common, and there are > many workarounds but tricky to explain; worst of all, I can't detect > the problem and warn. > > Cheers, > Sanne > > >> Cheers >> Manik >> >> -- >> Manik Surtani >> manik(a)jboss.org >> twitter.com/maniksurtani >> >> Lead, Infinispan >> http://www.infinispan.org >> >> >> >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Manik Surtani manik(a)jboss.org twitter.com/maniksurtani Lead, Infinispan http://www.infinispan.org _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Well I would not qualify that as a user error but that's definitely uncommon in a static language like Java. Though we will need to at least deal with sub/super classes change which are valid. On 27 mars 2012, at 16:56, Manik Surtani wrote: > I think it is fair to say that it is user error if the user changes the type of value of a key to one in a different index? You're right, detecting can be hard … > > On 27 Mar 2012, at 15:52, Sanne Grinovero wrote: > >> On 27 March 2012 15:37, Manik Surtani <manik(a)jboss.org&gt; wrote: >>> >>> On 27 Mar 2012, at 14:51, Sanne Grinovero wrote: >>> >>>> Hi all, >>>> I was adding some tests for Query as I just found many basic >>>> operations are uncovered, and found an annoying issue. >>>> >>>> When using >>>> >>>> cache.putAll(values); >>>> >>>> the Query interceptor will iterate on each entry, transform each entry >>>> key and replace those entry keys in string form with the new indexed >>>> values. >>>> >>>> The problem is *which index* : the index mapping information is bound >>>> to the indexed type. So the normal put/replace/remove operations >>>> actually rely on the return value to know where to direct the proper >>>> index operation (to which index) - if any. >>> >>> So even operations where return values are suppressed won't work? >> >> Depends, most cases will work anyway, but definitely some cases could >> misbehave when suppressing return values. >> I guess my next answers explain this better.. >> >> >>>> With putAll() returning void, I don't have the return values. >>>> >>>> I could hack this to "unroll" in several individual put() operations >>>> but I don't think that should happen in an interceptor. Also this >>>> would defeat the purpose of using putAll, killing it's efficiency over >>>> using several put operations. >>> >>> No, that's no good… >>> >>>> >>>> I would generally expect people to put objects of the same type when >>>> replacing a key.. so this works fine usually looking at the runtime >>>> type of the new value (as it's doing), but it's not correct generally. >>>> Worse, I can't detect the mismatch and log some warning. >>> >>> Why is that not correct? IMO, working on the type of the new value is more correct than doing so on the old value … >> >> Yes I definitely need the type of the new value to index the one one; >> but I also need the type of the previous value if it existed, to know >> how to cleanup existing index entries. >> >> >>>> Any suggestion? I'm tempted to say I need to retrieve all previous >>>> values. I actually need only the types :-/ >>>> >>>> A simple solution would be to disallow using multiple indexes for the >>>> same cache, but that's not a welcome change. I'd prefer to throw an >>>> exception on putAll : not supported when indexing is enabled. >>> >>> You could also maintain a type map? Keys -> Type. But that's unnecessarily voluminous. What's wrong with working off the type of the new value? Are you concerned about the types changing and hence not being able to clean up stale indexes? >> >> Exactly: the problem is with cleanup of previous index entries. >> Granted I don't expect this to be common; personally I would favour an >> application design in which - for a specific key - the type of the >> value could be determined by the key. >> But if you expect to support this: >> >> put( "name one", new SomeStuff("value") ); >> put( "name one", new SomeDifferentStuff("value") ); >> >> then, iff SomeStuff and SomeDifferentStuff are using different index >> names A and B, the second put needs to know the type of the previous >> value to remote "name one" from the index A while adding the new value >> to index B. >> >> For this to work I'm "having a look" at the type of the return value of the put. >> >> So indeed if >> a) you're suppressing return values >> b) two different types are indexed on different indexes >> c) you're reusing the same key instance for a different type >> >> then you get in trouble. Of course that's not common, and there are >> many workarounds but tricky to explain; worst of all, I can't detect >> the problem and warn. >> >> Cheers, >> Sanne >> >> >>> Cheers >>> Manik >>> >>> -- >>> Manik Surtani >>> manik(a)jboss.org >>> twitter.com/maniksurtani >>> >>> Lead, Infinispan >>> http://www.infinispan.org >>> >>> >>> >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev(a)lists.jboss.org >>> https://lists.jboss.org/mailman/listinfo/infinispan-dev >> >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > -- > Manik Surtani > manik(a)jboss.org > twitter.com/maniksurtani > > Lead, Infinispan > http://www.infinispan.org > > > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev