Re: svn commit: r1222241 - /ofbiz/trunk/framework/entity/
Locked
Re: svn commit: r1222241 - /ofbiz/trunk/framework/entity/
 
|
I don't like this commit - it looks like an ugly hack to me. What is the
end result supposed to be? Or what is this supposed to do? -Adrian On 12/22/2011 2:14 PM, [hidden email] wrote: > Author: jleroux > Date: Thu Dec 22 14:14:57 2011 > New Revision: 1222241 > > URL: http://svn.apache.org/viewvc?rev=1222241&view=rev > Log: > A modified patch from Patrick Antivackis "Null values are not synchronized in http mode" https://issues.apache.org/jira/browse/OFBIZ-4602 > > In order to send over http the values to create, store and remove, Ofbiz is Xml serializing the values. GenericValue xml serialization is managed in GenericEntity.makeXmlElement, unfortunately this method just don't serialized null valued fields. > To solve this issue, I managed null value the same way GenericEntity.setString (which is used for the Xml deserializing). I only managed until case 10, because i'm not sure the setString for the cases 11 to 15 are well managed (not taking care of null value) > > jleroux: After looking at GenericEntity.setString and initial commit (http://svn.ofbiz.org/viewcvs?rev=7779&view=rev) I see no reasons to not handling cases under 10 the same way. So I added these cases as well using fall through. > > Modified: > ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java > > Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java?rev=1222241&r1=1222240&r2=1222241&view=diff > ============================================================================== > --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java (original) > +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java Thu Dec 22 14:14:57 2011 > @@ -1077,6 +1077,42 @@ public class GenericEntity extends Obser > element.setAttribute(name, value); > } > } > + else { > + ModelFieldType type = null; > + try { > + type = getDelegator().getEntityFieldType(getModelEntity(), modelField.getType()); > + } catch (GenericEntityException e) { > + Debug.logWarning(e, module); > + } > + if (type == null) throw new IllegalArgumentException("Type " + modelField.getType() + " not found"); > + String fieldType = type.getJavaType(); > + > + try { > + switch (SqlJdbcUtil.getType(fieldType)) { > + case 1: // String > + set(name, ""); > + break; > + case 2: // Timestamp > + case 3: // Time > + case 4: // java.sql.Date > + case 5: // Integer > + case 6: // Long > + case 7: // Float > + case 8: // Double > + case 9: // BigDecimal > + case 10:// Boolean > + case 11:// Object > + case 12:// Blob, byte[], ByteBuffer, HeapByteBuffer > + case 13:// Clob > + case 14:// java.util.Date > + case 15:// Collection: ArrayList, HashSet, LinkedHashSet, LinkedList > + element.setAttribute(name, "null"); > + break; > + } > + } catch (GenericNotImplementedException ex) { > + throw new IllegalArgumentException(ex.getMessage()); > + } > + } > } > > return element; > > |
Re: svn commit: r1222241 - /ofbiz/trunk/framework/entity/
Administrator
|
Patrick crossed issues with Entity sync and null values and needed to address them. I trust his work.
Why does "it looks like an ugly hack" to you, a feeling, or? Do you expect some surprises with this work? How would you address the problem of null values, then ? Jacques From: "Adrian Crum" <[hidden email]> >I don't like this commit - it looks like an ugly hack to me. What is the end result supposed to be? Or what is this supposed to do? > > -Adrian > > On 12/22/2011 2:14 PM, [hidden email] wrote: >> Author: jleroux >> Date: Thu Dec 22 14:14:57 2011 >> New Revision: 1222241 >> >> URL: http://svn.apache.org/viewvc?rev=1222241&view=rev >> Log: >> A modified patch from Patrick Antivackis "Null values are not synchronized in http mode" >> https://issues.apache.org/jira/browse/OFBIZ-4602 >> >> In order to send over http the values to create, store and remove, Ofbiz is Xml serializing the values. GenericValue xml >> serialization is managed in GenericEntity.makeXmlElement, unfortunately this method just don't serialized null valued fields. >> To solve this issue, I managed null value the same way GenericEntity.setString (which is used for the Xml deserializing). I only >> managed until case 10, because i'm not sure the setString for the cases 11 to 15 are well managed (not taking care of null value) >> >> jleroux: After looking at GenericEntity.setString and initial commit (http://svn.ofbiz.org/viewcvs?rev=7779&view=rev) I see no >> reasons to not handling cases under 10 the same way. So I added these cases as well using fall through. >> >> Modified: >> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >> >> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >> URL: >> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java?rev=1222241&r1=1222240&r2=1222241&view=diff >> ============================================================================== >> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java (original) >> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java Thu Dec 22 14:14:57 2011 >> @@ -1077,6 +1077,42 @@ public class GenericEntity extends Obser >> element.setAttribute(name, value); >> } >> } >> + else { >> + ModelFieldType type = null; >> + try { >> + type = getDelegator().getEntityFieldType(getModelEntity(), modelField.getType()); >> + } catch (GenericEntityException e) { >> + Debug.logWarning(e, module); >> + } >> + if (type == null) throw new IllegalArgumentException("Type " + modelField.getType() + " not found"); >> + String fieldType = type.getJavaType(); >> + >> + try { >> + switch (SqlJdbcUtil.getType(fieldType)) { >> + case 1: // String >> + set(name, ""); >> + break; >> + case 2: // Timestamp >> + case 3: // Time >> + case 4: // java.sql.Date >> + case 5: // Integer >> + case 6: // Long >> + case 7: // Float >> + case 8: // Double >> + case 9: // BigDecimal >> + case 10:// Boolean >> + case 11:// Object >> + case 12:// Blob, byte[], ByteBuffer, HeapByteBuffer >> + case 13:// Clob >> + case 14:// java.util.Date >> + case 15:// Collection: ArrayList, HashSet, LinkedHashSet, LinkedList >> + element.setAttribute(name, "null"); >> + break; >> + } >> + } catch (GenericNotImplementedException ex) { >> + throw new IllegalArgumentException(ex.getMessage()); >> + } >> + } >> } >> >> return element; >> >> |
Re: svn commit: r1222241 - /ofbiz/trunk/framework/entity/
|
|
It looks like a hack because it has a long switch statement that doesn't
do anything, and in one condition the set method is being called while in another condition an element's set method is being called. In other words, this could be easily expressed as: element.setAttribute(name, value == null ? "null" : value); -Adrian On 12/22/2011 6:12 PM, Jacques Le Roux wrote: > Patrick crossed issues with Entity sync and null values and needed to > address them. I trust his work. > Why does "it looks like an ugly hack" to you, a feeling, or? Do you > expect some surprises with this work? How would you address the > problem of null values, then ? > > Jacques > > From: "Adrian Crum" <[hidden email]> >> I don't like this commit - it looks like an ugly hack to me. What is >> the end result supposed to be? Or what is this supposed to do? >> >> -Adrian >> >> On 12/22/2011 2:14 PM, [hidden email] wrote: >>> Author: jleroux >>> Date: Thu Dec 22 14:14:57 2011 >>> New Revision: 1222241 >>> >>> URL: http://svn.apache.org/viewvc?rev=1222241&view=rev >>> Log: >>> A modified patch from Patrick Antivackis "Null values are not >>> synchronized in http mode" >>> https://issues.apache.org/jira/browse/OFBIZ-4602 >>> >>> In order to send over http the values to create, store and remove, >>> Ofbiz is Xml serializing the values. GenericValue xml serialization >>> is managed in GenericEntity.makeXmlElement, unfortunately this >>> method just don't serialized null valued fields. >>> To solve this issue, I managed null value the same way >>> GenericEntity.setString (which is used for the Xml deserializing). I >>> only managed until case 10, because i'm not sure the setString for >>> the cases 11 to 15 are well managed (not taking care of null value) >>> >>> jleroux: After looking at GenericEntity.setString and initial commit >>> (http://svn.ofbiz.org/viewcvs?rev=7779&view=rev) I see no reasons to >>> not handling cases under 10 the same way. So I added these cases as >>> well using fall through. >>> >>> Modified: >>> >>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>> >>> Modified: >>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>> URL: >>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java?rev=1222241&r1=1222240&r2=1222241&view=diff >>> ============================================================================== >>> >>> --- >>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>> (original) >>> +++ >>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>> Thu Dec 22 14:14:57 2011 >>> @@ -1077,6 +1077,42 @@ public class GenericEntity extends Obser >>> element.setAttribute(name, value); >>> } >>> } >>> + else { >>> + ModelFieldType type = null; >>> + try { >>> + type = >>> getDelegator().getEntityFieldType(getModelEntity(), >>> modelField.getType()); >>> + } catch (GenericEntityException e) { >>> + Debug.logWarning(e, module); >>> + } >>> + if (type == null) throw new >>> IllegalArgumentException("Type " + modelField.getType() + " not >>> found"); >>> + String fieldType = type.getJavaType(); >>> + >>> + try { >>> + switch (SqlJdbcUtil.getType(fieldType)) { >>> + case 1: // String >>> + set(name, ""); >>> + break; >>> + case 2: // Timestamp >>> + case 3: // Time >>> + case 4: // java.sql.Date >>> + case 5: // Integer >>> + case 6: // Long >>> + case 7: // Float >>> + case 8: // Double >>> + case 9: // BigDecimal >>> + case 10:// Boolean >>> + case 11:// Object >>> + case 12:// Blob, byte[], ByteBuffer, >>> HeapByteBuffer >>> + case 13:// Clob >>> + case 14:// java.util.Date >>> + case 15:// Collection: ArrayList, HashSet, >>> LinkedHashSet, LinkedList >>> + element.setAttribute(name, "null"); >>> + break; >>> + } >>> + } catch (GenericNotImplementedException ex) { >>> + throw new >>> IllegalArgumentException(ex.getMessage()); >>> + } >>> + } >>> } >>> >>> return element; >>> >>> |
Re: svn commit: r1222241 - /ofbiz/trunk/framework/entity/
|
Administrator
|
In reply to this post by Adrian Crum-3
Adrian,
Also I must say that I took my time to consider this contribution. For instance, I looked, of course, in related stuff in trunk, but also at http://svn.ofbiz.org/viewcvs?rev=7779&view=rev This in case you were worried about no care taken before commit (I'm in vacation and have enough time for that :o) Let me know if you see something really worrying you... Jacques From: "Jacques Le Roux" <[hidden email]> > Patrick crossed issues with Entity sync and null values and needed to address them. I trust his work. > Why does "it looks like an ugly hack" to you, a feeling, or? Do you expect some surprises with this work? How would you address > the problem of null values, then ? > > Jacques > > From: "Adrian Crum" <[hidden email]> >>I don't like this commit - it looks like an ugly hack to me. What is the end result supposed to be? Or what is this supposed to >>do? >> >> -Adrian >> >> On 12/22/2011 2:14 PM, [hidden email] wrote: >>> Author: jleroux >>> Date: Thu Dec 22 14:14:57 2011 >>> New Revision: 1222241 >>> >>> URL: http://svn.apache.org/viewvc?rev=1222241&view=rev >>> Log: >>> A modified patch from Patrick Antivackis "Null values are not synchronized in http mode" >>> https://issues.apache.org/jira/browse/OFBIZ-4602 >>> >>> In order to send over http the values to create, store and remove, Ofbiz is Xml serializing the values. GenericValue xml >>> serialization is managed in GenericEntity.makeXmlElement, unfortunately this method just don't serialized null valued fields. >>> To solve this issue, I managed null value the same way GenericEntity.setString (which is used for the Xml deserializing). I only >>> managed until case 10, because i'm not sure the setString for the cases 11 to 15 are well managed (not taking care of null >>> value) >>> >>> jleroux: After looking at GenericEntity.setString and initial commit (http://svn.ofbiz.org/viewcvs?rev=7779&view=rev) I see no >>> reasons to not handling cases under 10 the same way. So I added these cases as well using fall through. >>> >>> Modified: >>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>> >>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>> URL: >>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java?rev=1222241&r1=1222240&r2=1222241&view=diff >>> ============================================================================== >>> --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java (original) >>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java Thu Dec 22 14:14:57 2011 >>> @@ -1077,6 +1077,42 @@ public class GenericEntity extends Obser >>> element.setAttribute(name, value); >>> } >>> } >>> + else { >>> + ModelFieldType type = null; >>> + try { >>> + type = getDelegator().getEntityFieldType(getModelEntity(), modelField.getType()); >>> + } catch (GenericEntityException e) { >>> + Debug.logWarning(e, module); >>> + } >>> + if (type == null) throw new IllegalArgumentException("Type " + modelField.getType() + " not found"); >>> + String fieldType = type.getJavaType(); >>> + >>> + try { >>> + switch (SqlJdbcUtil.getType(fieldType)) { >>> + case 1: // String >>> + set(name, ""); >>> + break; >>> + case 2: // Timestamp >>> + case 3: // Time >>> + case 4: // java.sql.Date >>> + case 5: // Integer >>> + case 6: // Long >>> + case 7: // Float >>> + case 8: // Double >>> + case 9: // BigDecimal >>> + case 10:// Boolean >>> + case 11:// Object >>> + case 12:// Blob, byte[], ByteBuffer, HeapByteBuffer >>> + case 13:// Clob >>> + case 14:// java.util.Date >>> + case 15:// Collection: ArrayList, HashSet, LinkedHashSet, LinkedList >>> + element.setAttribute(name, "null"); >>> + break; >>> + } >>> + } catch (GenericNotImplementedException ex) { >>> + throw new IllegalArgumentException(ex.getMessage()); >>> + } >>> + } >>> } >>> >>> return element; >>> >>> > |
Re: svn commit: r1222241 - /ofbiz/trunk/framework/entity/
|
Administrator
|
In reply to this post by Adrian Crum-3
Ha, it's only a syntax concern?
Actually I don't plenty agree. The String does not use element.setAttribute() and I wanted to make clear what the others types are. Unlike Patrick, I used a fall through switch. Also if ever we need to distinguish cases later if will be easier (unlikely but anyway the doc. aspect lead me to this prefer this syntax) Jacques From: "Adrian Crum" <[hidden email]> > It looks like a hack because it has a long switch statement that doesn't do anything, and in one condition the set method is being > called while in another condition an element's set method is being called. > > In other words, this could be easily expressed as: > > element.setAttribute(name, value == null ? "null" : value); > > -Adrian > > > On 12/22/2011 6:12 PM, Jacques Le Roux wrote: >> Patrick crossed issues with Entity sync and null values and needed to address them. I trust his work. >> Why does "it looks like an ugly hack" to you, a feeling, or? Do you expect some surprises with this work? How would you address >> the problem of null values, then ? >> >> Jacques >> >> From: "Adrian Crum" <[hidden email]> >>> I don't like this commit - it looks like an ugly hack to me. What is the end result supposed to be? Or what is this supposed to >>> do? >>> >>> -Adrian >>> >>> On 12/22/2011 2:14 PM, [hidden email] wrote: >>>> Author: jleroux >>>> Date: Thu Dec 22 14:14:57 2011 >>>> New Revision: 1222241 >>>> >>>> URL: http://svn.apache.org/viewvc?rev=1222241&view=rev >>>> Log: >>>> A modified patch from Patrick Antivackis "Null values are not synchronized in http mode" >>>> https://issues.apache.org/jira/browse/OFBIZ-4602 >>>> >>>> In order to send over http the values to create, store and remove, Ofbiz is Xml serializing the values. GenericValue xml >>>> serialization is managed in GenericEntity.makeXmlElement, unfortunately this method just don't serialized null valued fields. >>>> To solve this issue, I managed null value the same way GenericEntity.setString (which is used for the Xml deserializing). I >>>> only managed until case 10, because i'm not sure the setString for the cases 11 to 15 are well managed (not taking care of null >>>> value) >>>> >>>> jleroux: After looking at GenericEntity.setString and initial commit (http://svn.ofbiz.org/viewcvs?rev=7779&view=rev) I see no >>>> reasons to not handling cases under 10 the same way. So I added these cases as well using fall through. >>>> >>>> Modified: >>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>>> >>>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>>> URL: >>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java?rev=1222241&r1=1222240&r2=1222241&view=diff >>>> ============================================================================== >>>> --- >>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java (original) >>>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java Thu Dec 22 14:14:57 2011 >>>> @@ -1077,6 +1077,42 @@ public class GenericEntity extends Obser >>>> element.setAttribute(name, value); >>>> } >>>> } >>>> + else { >>>> + ModelFieldType type = null; >>>> + try { >>>> + type = getDelegator().getEntityFieldType(getModelEntity(), modelField.getType()); >>>> + } catch (GenericEntityException e) { >>>> + Debug.logWarning(e, module); >>>> + } >>>> + if (type == null) throw new IllegalArgumentException("Type " + modelField.getType() + " not found"); >>>> + String fieldType = type.getJavaType(); >>>> + >>>> + try { >>>> + switch (SqlJdbcUtil.getType(fieldType)) { >>>> + case 1: // String >>>> + set(name, ""); >>>> + break; >>>> + case 2: // Timestamp >>>> + case 3: // Time >>>> + case 4: // java.sql.Date >>>> + case 5: // Integer >>>> + case 6: // Long >>>> + case 7: // Float >>>> + case 8: // Double >>>> + case 9: // BigDecimal >>>> + case 10:// Boolean >>>> + case 11:// Object >>>> + case 12:// Blob, byte[], ByteBuffer, HeapByteBuffer >>>> + case 13:// Clob >>>> + case 14:// java.util.Date >>>> + case 15:// Collection: ArrayList, HashSet, LinkedHashSet, LinkedList >>>> + element.setAttribute(name, "null"); >>>> + break; >>>> + } >>>> + } catch (GenericNotImplementedException ex) { >>>> + throw new IllegalArgumentException(ex.getMessage()); >>>> + } >>>> + } >>>> } >>>> >>>> return element; >>>> >>>> |
Re: svn commit: r1222241 - /ofbiz/trunk/framework/entity/
|
|
No, it is not a syntax concern. The commit does not make sense.
For a String, you are calling GenericEntity.set(name, ""); and for all other types you are calling: Element.setAttribute(name, "null"); Why should a String data type be treated any differently than other data types? A null value is a null value - it doesn't have a "type" other than "null". And no, an empty unused switch statement does NOT help document anything - all it does is add confusion. -Adrian On 12/22/2011 9:39 PM, Jacques Le Roux wrote: > Ha, it's only a syntax concern? > > Actually I don't plenty agree. The String does not use > element.setAttribute() and I wanted to make clear what the others > types are. Unlike Patrick, I used a fall through switch. Also if ever > we need to distinguish cases later if will be easier (unlikely but > anyway the doc. aspect lead me to this prefer this syntax) > > Jacques > > From: "Adrian Crum" <[hidden email]> >> It looks like a hack because it has a long switch statement that >> doesn't do anything, and in one condition the set method is being >> called while in another condition an element's set method is being >> called. >> >> In other words, this could be easily expressed as: >> >> element.setAttribute(name, value == null ? "null" : value); >> >> -Adrian >> >> >> On 12/22/2011 6:12 PM, Jacques Le Roux wrote: >>> Patrick crossed issues with Entity sync and null values and needed >>> to address them. I trust his work. >>> Why does "it looks like an ugly hack" to you, a feeling, or? Do you >>> expect some surprises with this work? How would you address the >>> problem of null values, then ? >>> >>> Jacques >>> >>> From: "Adrian Crum" <[hidden email]> >>>> I don't like this commit - it looks like an ugly hack to me. What >>>> is the end result supposed to be? Or what is this supposed to do? >>>> >>>> -Adrian >>>> >>>> On 12/22/2011 2:14 PM, [hidden email] wrote: >>>>> Author: jleroux >>>>> Date: Thu Dec 22 14:14:57 2011 >>>>> New Revision: 1222241 >>>>> >>>>> URL: http://svn.apache.org/viewvc?rev=1222241&view=rev >>>>> Log: >>>>> A modified patch from Patrick Antivackis "Null values are not >>>>> synchronized in http mode" >>>>> https://issues.apache.org/jira/browse/OFBIZ-4602 >>>>> >>>>> In order to send over http the values to create, store and remove, >>>>> Ofbiz is Xml serializing the values. GenericValue xml >>>>> serialization is managed in GenericEntity.makeXmlElement, >>>>> unfortunately this method just don't serialized null valued fields. >>>>> To solve this issue, I managed null value the same way >>>>> GenericEntity.setString (which is used for the Xml deserializing). >>>>> I only managed until case 10, because i'm not sure the setString >>>>> for the cases 11 to 15 are well managed (not taking care of null >>>>> value) >>>>> >>>>> jleroux: After looking at GenericEntity.setString and initial >>>>> commit (http://svn.ofbiz.org/viewcvs?rev=7779&view=rev) I see no >>>>> reasons to not handling cases under 10 the same way. So I added >>>>> these cases as well using fall through. >>>>> >>>>> Modified: >>>>> >>>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>>>> >>>>> Modified: >>>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>>>> URL: >>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java?rev=1222241&r1=1222240&r2=1222241&view=diff >>>>> ============================================================================== >>>>> >>>>> --- >>>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>>>> (original) >>>>> +++ >>>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>>>> Thu Dec 22 14:14:57 2011 >>>>> @@ -1077,6 +1077,42 @@ public class GenericEntity extends Obser >>>>> element.setAttribute(name, value); >>>>> } >>>>> } >>>>> + else { >>>>> + ModelFieldType type = null; >>>>> + try { >>>>> + type = >>>>> getDelegator().getEntityFieldType(getModelEntity(), >>>>> modelField.getType()); >>>>> + } catch (GenericEntityException e) { >>>>> + Debug.logWarning(e, module); >>>>> + } >>>>> + if (type == null) throw new >>>>> IllegalArgumentException("Type " + modelField.getType() + " not >>>>> found"); >>>>> + String fieldType = type.getJavaType(); >>>>> + >>>>> + try { >>>>> + switch (SqlJdbcUtil.getType(fieldType)) { >>>>> + case 1: // String >>>>> + set(name, ""); >>>>> + break; >>>>> + case 2: // Timestamp >>>>> + case 3: // Time >>>>> + case 4: // java.sql.Date >>>>> + case 5: // Integer >>>>> + case 6: // Long >>>>> + case 7: // Float >>>>> + case 8: // Double >>>>> + case 9: // BigDecimal >>>>> + case 10:// Boolean >>>>> + case 11:// Object >>>>> + case 12:// Blob, byte[], ByteBuffer, >>>>> HeapByteBuffer >>>>> + case 13:// Clob >>>>> + case 14:// java.util.Date >>>>> + case 15:// Collection: ArrayList, HashSet, >>>>> LinkedHashSet, LinkedList >>>>> + element.setAttribute(name, "null"); >>>>> + break; >>>>> + } >>>>> + } catch (GenericNotImplementedException ex) { >>>>> + throw new >>>>> IllegalArgumentException(ex.getMessage()); >>>>> + } >>>>> + } >>>>> } >>>>> >>>>> return element; >>>>> >>>>> |
Re: svn commit: r1222241 - /ofbiz/trunk/framework/entity/
|
Administrator
|
I must say, I was also surprised by the GenericEntity.set(name, "") for String. As I know it fixed an issue with Entity Sync, as I
discussed with Patrick, it made sense to me to commit as is (though I did not had a chance to test it, which would be rather complicated) For the fall through cases, I don't see them as "unused switch statements", simply fall through cases using the same (final) expression. They had an idea about which types correspond to cases, which I don't see as useless... Sorry it's my last answer on this, I will leave for vacation tomorrow morning... If you think it should be reverted, no pb with me, I will see that ater BTW, maybe the 1st case should be handled by element.setAttribute(name, "") (preferred to element.setAttribute(name, "null") not sure about that yet). But then I wonder why Patrick had to use set(name, ""). Because I know this is used in real world somewhere for Entity Sync success... I agree this needs to be clarified Jacques From: "Adrian Crum" <[hidden email]> > No, it is not a syntax concern. The commit does not make sense. > > For a String, you are calling > > GenericEntity.set(name, ""); > > and for all other types you are calling: > > Element.setAttribute(name, "null"); > > Why should a String data type be treated any differently than other data types? A null value is a null value - it doesn't have a > "type" other than "null". > > And no, an empty unused switch statement does NOT help document anything - all it does is add confusion. > > -Adrian > > On 12/22/2011 9:39 PM, Jacques Le Roux wrote: >> Ha, it's only a syntax concern? >> >> Actually I don't plenty agree. The String does not use element.setAttribute() and I wanted to make clear what the others types >> are. Unlike Patrick, I used a fall through switch. Also if ever we need to distinguish cases later if will be easier (unlikely >> but anyway the doc. aspect lead me to this prefer this syntax) >> >> Jacques >> >> From: "Adrian Crum" <[hidden email]> >>> It looks like a hack because it has a long switch statement that doesn't do anything, and in one condition the set method is >>> being called while in another condition an element's set method is being called. >>> >>> In other words, this could be easily expressed as: >>> >>> element.setAttribute(name, value == null ? "null" : value); >>> >>> -Adrian >>> >>> >>> On 12/22/2011 6:12 PM, Jacques Le Roux wrote: >>>> Patrick crossed issues with Entity sync and null values and needed to address them. I trust his work. >>>> Why does "it looks like an ugly hack" to you, a feeling, or? Do you expect some surprises with this work? How would you address >>>> the problem of null values, then ? >>>> >>>> Jacques >>>> >>>> From: "Adrian Crum" <[hidden email]> >>>>> I don't like this commit - it looks like an ugly hack to me. What is the end result supposed to be? Or what is this supposed >>>>> to do? >>>>> >>>>> -Adrian >>>>> >>>>> On 12/22/2011 2:14 PM, [hidden email] wrote: >>>>>> Author: jleroux >>>>>> Date: Thu Dec 22 14:14:57 2011 >>>>>> New Revision: 1222241 >>>>>> >>>>>> URL: http://svn.apache.org/viewvc?rev=1222241&view=rev >>>>>> Log: >>>>>> A modified patch from Patrick Antivackis "Null values are not synchronized in http mode" >>>>>> https://issues.apache.org/jira/browse/OFBIZ-4602 >>>>>> >>>>>> In order to send over http the values to create, store and remove, Ofbiz is Xml serializing the values. GenericValue xml >>>>>> serialization is managed in GenericEntity.makeXmlElement, unfortunately this method just don't serialized null valued fields. >>>>>> To solve this issue, I managed null value the same way GenericEntity.setString (which is used for the Xml deserializing). I >>>>>> only managed until case 10, because i'm not sure the setString for the cases 11 to 15 are well managed (not taking care of >>>>>> null value) >>>>>> >>>>>> jleroux: After looking at GenericEntity.setString and initial commit (http://svn.ofbiz.org/viewcvs?rev=7779&view=rev) I see >>>>>> no reasons to not handling cases under 10 the same way. So I added these cases as well using fall through. >>>>>> >>>>>> Modified: >>>>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>>>>> >>>>>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>>>>> URL: >>>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java?rev=1222241&r1=1222240&r2=1222241&view=diff >>>>>> ============================================================================== >>>>>> --- >>>>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java (original) >>>>>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java Thu Dec 22 14:14:57 2011 >>>>>> @@ -1077,6 +1077,42 @@ public class GenericEntity extends Obser >>>>>> element.setAttribute(name, value); >>>>>> } >>>>>> } >>>>>> + else { >>>>>> + ModelFieldType type = null; >>>>>> + try { >>>>>> + type = getDelegator().getEntityFieldType(getModelEntity(), modelField.getType()); >>>>>> + } catch (GenericEntityException e) { >>>>>> + Debug.logWarning(e, module); >>>>>> + } >>>>>> + if (type == null) throw new IllegalArgumentException("Type " + modelField.getType() + " not found"); >>>>>> + String fieldType = type.getJavaType(); >>>>>> + >>>>>> + try { >>>>>> + switch (SqlJdbcUtil.getType(fieldType)) { >>>>>> + case 1: // String >>>>>> + set(name, ""); >>>>>> + break; >>>>>> + case 2: // Timestamp >>>>>> + case 3: // Time >>>>>> + case 4: // java.sql.Date >>>>>> + case 5: // Integer >>>>>> + case 6: // Long >>>>>> + case 7: // Float >>>>>> + case 8: // Double >>>>>> + case 9: // BigDecimal >>>>>> + case 10:// Boolean >>>>>> + case 11:// Object >>>>>> + case 12:// Blob, byte[], ByteBuffer, HeapByteBuffer >>>>>> + case 13:// Clob >>>>>> + case 14:// java.util.Date >>>>>> + case 15:// Collection: ArrayList, HashSet, LinkedHashSet, LinkedList >>>>>> + element.setAttribute(name, "null"); >>>>>> + break; >>>>>> + } >>>>>> + } catch (GenericNotImplementedException ex) { >>>>>> + throw new IllegalArgumentException(ex.getMessage()); >>>>>> + } >>>>>> + } >>>>>> } >>>>>> >>>>>> return element; >>>>>> >>>>>> |
Re: svn commit: r1222241 - /ofbiz/trunk/framework/entity/
|
Administrator
|
I finaly reverted because of https://issues.apache.org/jira/browse/OFBIZ-4637.
I guess only a 1st detected symptom I will see later what the problem was and will try to redo Patrick's work, it's maybe just because set(name, ""); was used, instead element.setAttribute(name, "null"); Jacques From: "Jacques Le Roux" <[hidden email]> >I must say, I was also surprised by the GenericEntity.set(name, "") for String. As I know it fixed an issue with Entity Sync, as I >discussed with Patrick, it made sense to me to commit as is (though I did not had a chance to test it, which would be rather >complicated) > > For the fall through cases, I don't see them as "unused switch statements", simply fall through cases using the same (final) > expression. They had an idea about which types correspond to cases, which I don't see as useless... > > Sorry it's my last answer on this, I will leave for vacation tomorrow morning... > If you think it should be reverted, no pb with me, I will see that ater > > BTW, maybe the 1st case should be handled by element.setAttribute(name, "") (preferred to element.setAttribute(name, "null") not > sure about that yet). But then I wonder why Patrick had to use set(name, ""). Because I know this is used in real world somewhere > for Entity Sync success... > > I agree this needs to be clarified > > Jacques > > From: "Adrian Crum" <[hidden email]> >> No, it is not a syntax concern. The commit does not make sense. >> >> For a String, you are calling >> >> GenericEntity.set(name, ""); >> >> and for all other types you are calling: >> >> Element.setAttribute(name, "null"); >> >> Why should a String data type be treated any differently than other data types? A null value is a null value - it doesn't have a >> "type" other than "null". >> >> And no, an empty unused switch statement does NOT help document anything - all it does is add confusion. >> >> -Adrian >> >> On 12/22/2011 9:39 PM, Jacques Le Roux wrote: >>> Ha, it's only a syntax concern? >>> >>> Actually I don't plenty agree. The String does not use element.setAttribute() and I wanted to make clear what the others types >>> are. Unlike Patrick, I used a fall through switch. Also if ever we need to distinguish cases later if will be easier (unlikely >>> but anyway the doc. aspect lead me to this prefer this syntax) >>> >>> Jacques >>> >>> From: "Adrian Crum" <[hidden email]> >>>> It looks like a hack because it has a long switch statement that doesn't do anything, and in one condition the set method is >>>> being called while in another condition an element's set method is being called. >>>> >>>> In other words, this could be easily expressed as: >>>> >>>> element.setAttribute(name, value == null ? "null" : value); >>>> >>>> -Adrian >>>> >>>> >>>> On 12/22/2011 6:12 PM, Jacques Le Roux wrote: >>>>> Patrick crossed issues with Entity sync and null values and needed to address them. I trust his work. >>>>> Why does "it looks like an ugly hack" to you, a feeling, or? Do you expect some surprises with this work? How would you >>>>> address the problem of null values, then ? >>>>> >>>>> Jacques >>>>> >>>>> From: "Adrian Crum" <[hidden email]> >>>>>> I don't like this commit - it looks like an ugly hack to me. What is the end result supposed to be? Or what is this supposed >>>>>> to do? >>>>>> >>>>>> -Adrian >>>>>> >>>>>> On 12/22/2011 2:14 PM, [hidden email] wrote: >>>>>>> Author: jleroux >>>>>>> Date: Thu Dec 22 14:14:57 2011 >>>>>>> New Revision: 1222241 >>>>>>> >>>>>>> URL: http://svn.apache.org/viewvc?rev=1222241&view=rev >>>>>>> Log: >>>>>>> A modified patch from Patrick Antivackis "Null values are not synchronized in http mode" >>>>>>> https://issues.apache.org/jira/browse/OFBIZ-4602 >>>>>>> >>>>>>> In order to send over http the values to create, store and remove, Ofbiz is Xml serializing the values. GenericValue xml >>>>>>> serialization is managed in GenericEntity.makeXmlElement, unfortunately this method just don't serialized null valued >>>>>>> fields. >>>>>>> To solve this issue, I managed null value the same way GenericEntity.setString (which is used for the Xml deserializing). I >>>>>>> only managed until case 10, because i'm not sure the setString for the cases 11 to 15 are well managed (not taking care of >>>>>>> null value) >>>>>>> >>>>>>> jleroux: After looking at GenericEntity.setString and initial commit (http://svn.ofbiz.org/viewcvs?rev=7779&view=rev) I see >>>>>>> no reasons to not handling cases under 10 the same way. So I added these cases as well using fall through. >>>>>>> >>>>>>> Modified: >>>>>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>>>>>> >>>>>>> Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java >>>>>>> URL: >>>>>>> http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java?rev=1222241&r1=1222240&r2=1222241&view=diff >>>>>>> ============================================================================== >>>>>>> --- >>>>>>> ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java (original) >>>>>>> +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericEntity.java Thu Dec 22 14:14:57 2011 >>>>>>> @@ -1077,6 +1077,42 @@ public class GenericEntity extends Obser >>>>>>> element.setAttribute(name, value); >>>>>>> } >>>>>>> } >>>>>>> + else { >>>>>>> + ModelFieldType type = null; >>>>>>> + try { >>>>>>> + type = getDelegator().getEntityFieldType(getModelEntity(), modelField.getType()); >>>>>>> + } catch (GenericEntityException e) { >>>>>>> + Debug.logWarning(e, module); >>>>>>> + } >>>>>>> + if (type == null) throw new IllegalArgumentException("Type " + modelField.getType() + " not found"); >>>>>>> + String fieldType = type.getJavaType(); >>>>>>> + >>>>>>> + try { >>>>>>> + switch (SqlJdbcUtil.getType(fieldType)) { >>>>>>> + case 1: // String >>>>>>> + set(name, ""); >>>>>>> + break; >>>>>>> + case 2: // Timestamp >>>>>>> + case 3: // Time >>>>>>> + case 4: // java.sql.Date >>>>>>> + case 5: // Integer >>>>>>> + case 6: // Long >>>>>>> + case 7: // Float >>>>>>> + case 8: // Double >>>>>>> + case 9: // BigDecimal >>>>>>> + case 10:// Boolean >>>>>>> + case 11:// Object >>>>>>> + case 12:// Blob, byte[], ByteBuffer, HeapByteBuffer >>>>>>> + case 13:// Clob >>>>>>> + case 14:// java.util.Date >>>>>>> + case 15:// Collection: ArrayList, HashSet, LinkedHashSet, LinkedList >>>>>>> + element.setAttribute(name, "null"); >>>>>>> + break; >>>>>>> + } >>>>>>> + } catch (GenericNotImplementedException ex) { >>>>>>> + throw new IllegalArgumentException(ex.getMessage()); >>>>>>> + } >>>>>>> + } >>>>>>> } >>>>>>> >>>>>>> return element; >>>>>>> >>>>>>> |
«
Return to OFBiz - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |