[QFJ-962] FieldMap.setField(int key, Field<?> field) does not properly convert values Created: 15/Nov/18  Updated: 15/Jun/20

Status: Open
Project: QuickFIX/J
Component/s: Engine
Affects Version/s: 2.1.0
Fix Version/s: 3.0.0

Type: Bug Priority: Default
Reporter: amichair Assignee: Wojciech Zankowski
Resolution: Unresolved Votes: 0
Labels: None

Issue Links:
Relates
relates to QFJ-428 FieldMap.setField(int key, Field<?> f... Closed

 Description   

QFJ-428 made the generic setField(int key, Field<?> field) method public, claiming this will avoid having to use a switch statement and call each type-specific overload explicitly.

However, this is misleading and introduces a bug, since the two are not equivalent.

Specifically, the type-specific overloads do proper type conversions on the value, while the generic setField does not. For example, if you pass a DoubleField with a very large or small value, it will convert it into a string in scientific notation (with exponent) which is invalid in the FIX protocol, whereas calling the type-specific setField(DoubleField field) will properly use the DoubleConverter to convert it correctly.

When this method was non-public this was less of a problem, assuming its internal usages are correct (I don't know if this is the case, but assume it is). However making it public now creates a pitfall for anyone using this method in client code which will likely result in a bug on some data.

The solution would be to either implement it fully with proper type conversion (e.g. using the switch statement from QFJ-428), or at the very least, to add a javadoc to this method that warns about using it and clearly states that it does not do proper type conversion, so users will be able to decide whether this is what they really intend to do or not.

 Comments   
Comment by Christoph John [ 16/Nov/18 ]

Hi amichair,

maybe we should add the javadoc comment as you suggested and provide an additional method, say setFieldConverted(int key, Field<?> field) , so users can choose which method suits them best.

Cheers,
Chris.

Comment by Christoph John [ 16/Nov/18 ]

Briefly had a look at this. What about making Field abstract (According to a comment in the class it is only non-abstract to have JNI compatibility. But that was like 10 years ago.) and adding an abstract convertToString() method that does the necessary conversion in the subclasses? Example for BooleanField:

    @Override
    public String convertToString() {
        return BooleanConverter.convert(getValue());
    }

Then we can call in FieldMap:

    public void setField(int key, Field<?> field) {
        setString(key, field.convertToString());
    }

That way we can ensure that we always have a conversion method for a field and don't need a lengthy if-else statement.

What do you think?

Comment by amichair [ 22/Nov/18 ]

Encapsulating the converters inside the Field classes sounds like a good idea regardless of this issue or of Field being abstract

I have't delved into this one yet, but perhaps instead of convertToString, maybe toString itself should just do this properly?

Comment by Wojciech Zankowski [ 29/Jul/19 ]

Pull request created with implementation of the idea: https://github.com/quickfix-j/quickfixj/pull/226

Generated at Fri May 03 10:41:42 UTC 2024 using JIRA 7.5.2#75007-sha1:9f5725bb824792b3230a5d8716f0c13e296a3cae.