Bug in encode functions of FieldInstructionAscii

[GoogleCodeExporter]

Example of encodeCopy:

    if(isMandatory())
    {
      encoder.reportFatal("[ERR U01]", "Missing mandatory field.", *identity_);
      // if reportFatal returns we're being lax about the rules
      // let the copy happen.
      pmap.setNextField(false);
    }
    if(previousStatus == Context::OK_VALUE)
    {
      // we have to null the previous value to avoid copy
      pmap.setNextField(true);// value in stream
      destination.putByte(nullAscii);
    }
    else
    {
      pmap.setNextField(false);
    }
    if(previousStatus != Context::NULL_VALUE)
    {
      fieldOp_->setDictionaryValueNull(encoder);
    }

There is a bug in FieldInstructionAscii for optional fields with initial
value. If the field is not present in the fieldset, the pmap bit will set
to 0 while its behavior should be:

-Set 1 in the PMAP. 
-Put a NULL byte in the stream.
-Update the dictionary to NULL.

To fix this the easy way is to use the code of FieldInstructionInteger
where this bug is not present:

        if(isMandatory())
        {
          encoder.reportError("[ERR U01]", "Missing mandatory integer
field.", *identity_);
          // if reportEror returns we're being lax about the rules.
          // Let the copy happen
          pmap.setNextField(false);
        }
        else
        {
          // Missing optional field.  If we have a previous, non-null value
          // we need to explicitly null it out.  Otherwise just don't send it.
          if(previousStatus != Context::NULL_VALUE)
          {
            pmap.setNextField(true);// value in stream
            destination.putByte(nullInteger);
            fieldOp_->setDictionaryValueNull(encoder);
          }
          else
          {
            pmap.setNextField(false);
          }
        }

Original issue reported on code.google.com by carlo...@gmail.com on 12 Apr 2010 at 2:19

[GoogleCodeExporter]

Thanks, Carlos.
Fixed in R388.

Original comment by dale.wil...@gmail.com on 12 Apr 2010 at 10:34

• Changed state: Fixed

[GoogleCodeExporter]

Hello Dale,

As always thanks for your "QUICKFAST" response.

I've been reviewing your fix and I've seen you have modified the following line:

Original -> if(previousStatus != Context::NULL_VALUE)

Fix R388 -> if(previousStatus != Context::NULL_VALUE && previousStatus !=
Context::UNDEFINED_VALUE)

I think the correct instruction would be the original. Let me show you an 
example:

Let's suppose that we have the following template:

<template name="XXXX">
  <uint32 name="FIELD" presence='optional'><copy value='2'/></uint32>
</template>

Now, we want let FIELD unset, if I understand the FAST specifications(many 
times I
don't), FIELD should be encoded as NULL byte and PMAP as true. But, instead:

1) The dictionary will be updated by line: fieldOp_->setDictionaryValue(encoder,
previousValue);. Note that the dictionary is updated, but previousStatus 
variable is
still Context::UNDEFINED_VALUE

2) Now, because previousStatus doesn't satisfy this condition: 
  if(previousStatus != Context::NULL_VALUE && previousStatus != Context::UNDEFINED_VALUE)

Hereby, the only instruction that will be executed is: pmap.setNextField(false);

Original comment by carlo...@gmail.com on 12 Apr 2010 at 11:05

[GoogleCodeExporter]

One more pass at fixing this one completely:  Revised the Ascii and Integer 
fixes to 
handle all combinations of default value/no default value and 
mandatory/optional. (With 
a new unit test in testFieldOperations.)

Also applied the same fixes to FieldInstructionDecimal and FieldInstructionBlob 
(UTF8 
and ByteVector)

Original comment by dale.wil...@gmail.com on 13 Apr 2010 at 9:11