do not lookup field types in FormFieldRegistry without a FORM_TYPE

[fenuz]

The FormFieldRegistry looks up the FORM_TYPE for a field if it is not specified. This can cause unpredictable results when a form uses a field name also used by another (registered) form.

This PR ensures that forms without a FORM_TYPE do not lookup field types. Instead the default text-single is used.

[Flowdalic]

That should be a else branch in the if/else above

[fenuz]

Committed change to make it so. However it does result in a small duplication of the code setting the default. An alternative would be the replacing the else with another if (type == null), but inside the initial if (type == null) block. That would remove the duplication, but prevent a double (type == null) check when a type was set in the first place.

[Flowdalic]

The FormFieldRegistry looks up the FORM_TYPE for a field if it is not specified. This can cause unpredictable results when a form uses a field name also used by another (registered) form.

This PR ensures that forms without a FORM_TYPE do not lookup field types. Instead the default text-single is used.

This should go into the commit message

[fenuz]

The FormFieldRegistry looks up the FORM_TYPE for a field if it is not specified. This can cause unpredictable results when a form uses a field name also used by another (registered) form.
This PR ensures that forms without a FORM_TYPE do not lookup field types. Instead the default text-single is used.

This should go into the commit message

force pushed the changes with a new commit message

[Flowdalic]

I am sorry, but we where both wrong, this should be if (formType != null) {, then lines 220-221 can be removed.

[fenuz]

I don't think I understand. If the type is null we should do the following:

1. if its a FORM_TYPE field we set the type to hidden (this might not be completely correct, the XEP-0068 describes this behavior for forms of type submit, for result and form it writes: "If the FORM_TYPE field is not hidden in a form with type="form" or type="result", it MUST be ignored as a context indicator.")
2. if a FORM_TYPE was set, we lookup the field type in the FormFieldRegistry
3. if still no type is known, we use the default described in XEP-0004 text-single.

I think the current code implements this?

[Flowdalic]

Sorry, I wrote formType, when I meant type (the field's type).

            } else if (formType != null) {
                // If no field type was explicitly provided, then we need to lookup the
                // field's type in the registry.
                type = FormFieldRegistry.lookup(formType, fieldName);
                if (type == null) {
                    LOGGER.warning("The Field '" + fieldName + "' from FORM_TYPE '" + formType
                                    + "' is not registered. Field type is unknown, assuming text-single.");
                    // As per XEP-0004, text-single is the default form field type, which we use as emergency fallback here.
                    type = FormField.Type.text_single;
                }
            } else {
               // As per XEP-0004, text-single is the default form field type
               type = FormField.Type.text_single;
            }

should be

            } else if (formType != null) {
                // If no field type was explicitly provided, then we need to lookup the
                // field's type in the registry.
                type = FormFieldRegistry.lookup(formType, fieldName);
                if (type == null) {
                    LOGGER.warning("The Field '" + fieldName + "' from FORM_TYPE '" + formType
                                    + "' is not registered. Field type is unknown, assuming text-single.");
                }
            }

            if (type != null) {
               // As per XEP-0004, text-single is the default form field type
               type = FormField.Type.text_single;
            }

[Flowdalic]

FYI, I have pushed to the destination branch. Especially igniterealtime@97a5cc7 is probably would I would prefer to this PR, as I think it would probably solve the issues you are seeing while still keeping that part of the registry.

Could you test and report back? Thanks!

[fenuz]

FYI, I have pushed to the destination branch. Especially igniterealtime@97a5cc7 is probably would I would prefer to this PR, as I think it would probably solve the issues you are seeing while still keeping that part of the registry.

Could you test and report back? Thanks!

Thank you for your support! :)

This would prevent custom form fields using clark notation being registered in the FormFieldRegistry. It would indeed provide us a work around for our problems, by renaming our fields to be in clark notation.

However I am not sure I like this solution much, for the following reasons:

• The Field Names section in XEP-0068, detailing the requirement for Clark Notation, apply to custom field names in forms that have a registered FORM_TYPE set. Not for forms without a FORM_TYPE, or for unregistered FORM_TYPEs. The check for Clark Notation during field type lookup does not seem supported by any requirement in XEP-0068.
• The Clark Notation requirement is intended to prevent field name collisions between unofficial/unregistered fields in registered forms. It would still be useful if the FormFieldRegistry could lookup the type of a unofficial field, and the clark notation actually makes this feasible.
• Our forms do not have a FORM_TYPE, and will not be registered by the DataForm class. A lookup, regardless of the fieldName, should not return anything.
• The current implementation will do a lookup, and might return a type, for non-Clark Notation fields in forms without a FORM_TYPE. My interpretation would be that fields in a form without a FORM_TYPE should not be looked up.

So I think the clark notation check gives us a workaround: a way to prevent field type lookup. But I feel that it would be more elegant if no lookup is done for fields in forms without a FORM_TYPE. This would better adhere to the XEP's (4, 68), and would make the registry and type resolving very predicable and consistent.

What do you think?

[Flowdalic]

I found that my commit used if (XmlUtil.isClarkNotation(fieldName)), when I intended to write if (!XmlUtil.isClarkNotation(fieldName). This probably caused a lot of confusion, for which I am sorry.

Anyway, I created another attempt in
igniterealtime#464
which is an alternative to the previous one (igniterealtime#462). Happy to hear your thoughts on this.

[fenuz]

I found that my commit used if (XmlUtil.isClarkNotation(fieldName)), when I intended to write if (!XmlUtil.isClarkNotation(fieldName). This probably caused a lot of confusion, for which I am sorry.

Anyway, I created another attempt in
igniterealtime#464
which is an alternative to the previous one (igniterealtime#462). Happy to hear your thoughts on this.

Hehe, yes this seems a lot better. It registers all forms, regardless of FORM_TYPE. But for forms without FORM_TYPE it will only register fields using clark notation. Likewise the lookup for forms without FORM_TYPE only happens on fields using clark notation.
I think this solves our issues, and is pretty neat as it still allows for field type lookup even on forms without FORM_TYPE if the fieldnames use clark notation. Nice!

I think FormFieldRegistry.java lines 101 to 103 can be removed though, as the CLARK_NOTATION_FIELD_REGISTRY would already return null. But it does make the intent of the code a bit more obvious.

[Flowdalic]

I think FormFieldRegistry.java lines 101 to 103 can be removed though, as the CLARK_NOTATION_FIELD_REGISTRY would already return null. But it does make the intent of the code a bit more obvious.

It furthermore avoid access to a synchronized datastructure when you know that there will be no data in the structure.

[fenuz]

I think FormFieldRegistry.java lines 101 to 103 can be removed though, as the CLARK_NOTATION_FIELD_REGISTRY would already return null. But it does make the intent of the code a bit more obvious.

It furthermore avoid access to a synchronized datastructure when you know that there will be no data in the structure.

Good point.

So I am pretty happy with this solution, it solves our problem of loading forms without a FORM_TYPE.

Could you also consider the changes in cba0948

This would allow us to use the Filled/FillableForm API for forms without a FORM_TYPE.

[Flowdalic]

Could you also consider the changes in cba0948

I will consider them. I have not had yet time to think about this. My plan was to get a 4.4.1 release out the door and then think about this.