bindData with empty include list includes everything

The original description here says "but nothing should be bound". Is that really correct? That isn't how the Spring DataBinder class works. It says that if allowedFields is empty then a field is allowed as long as it isn't included in the explicit excludes.

        ...

	protected boolean isAllowed(String field) {
		String[] allowed = getAllowedFields();
		String[] disallowed = getDisallowedFields();
		return ((ObjectUtils.isEmpty(allowed) || PatternMatchUtils.simpleMatch(allowed, field)) &&
				(ObjectUtils.isEmpty(disallowed) || !PatternMatchUtils.simpleMatch(disallowed, field)));
	}

        ...

We can circumvent this by adding logic of our own in GrailsDataBinder but I am not sure that is the right thing to do.

Should we change this behavior or leave it as is?

The "nothing should be bound" is my naive expectation when, e.g., reducing the list from one property to no properties. I'm surprised if Spring DataBinder is supposed to work that way, although I see that's how it's implemented (using isEmpty() instead of allowed == null). Nevertheless, I don't see anything in its javadoc that suggests that setting the allowed fields to an array of no fields is the same as setting it to the default of all fields. (On the other hand, I'd expect that setting it back to null would be the default, but the javadoc doesn't mention null either.)

	/**
	 * Register fields that should be allowed for binding. Default is all
	 * fields. Restrict this for example to avoid unwanted modifications
	 * by malicious users when binding HTTP request parameters.
	 * <p>Supports "xxx*", "*xxx" and "*xxx*" patterns. More sophisticated matching
	 * can be implemented by overriding the <code>isAllowed</code> method.
	 * <p>Alternatively, specify a list of <i>disallowed</i> fields.
	 * @param allowedFields array of field names
	 * @see #setDisallowedFields
	 * @see #isAllowed(String)
	 * @see org.springframework.web.bind.ServletRequestDataBinder
	 */
	public void setAllowedFields(String... allowedFields) {
		this.allowedFields = PropertyAccessorUtils.canonicalPropertyNames(allowedFields);
	}

	/**
	 * Return the fields that should be allowed for binding.
	 * @return array of field names
	 */
	public String[] getAllowedFields() {
		return this.allowedFields;
	}

Anyway, regardless of how Spring is supposed to work, I think that Grails would be more secure and less surprising if, when given a list of properties to bind, the list is not ignored when its size goes down to 0.

By the way, isEmpty() is correct for disallowed. I think it's just a bug in DataBinder to use isEmpty() for allowed too.

Note that canonicalPropertyNames(String[]) preserves the distinction between null and the empty array.

It seems a little perverse to want to include no properties during the binding. Effectively the binding doesn't happen. However, I agree with the logic that an empty list means don't include any properties. If someone passes in an empty list, we should honour that even if it seems odd.

On the other hand, it would be a breaking change that could adversely affect existing applications. Is the current behaviour going to affect enough people to warrant a breaking change? I'm not sure.

I'm currently leaning towards leaving it as is and documenting this corner case. Perhaps if data binding undergoes a significant overhaul, then we can revisit this.

My usage was a list of property names filtered by authorization. So this issue is the case where the user isn't authorized to update any of the fields on that part of the domain instance.

Of course, now that I have discovered that the empty list is a special case, it's easy for me to work around it. Regarding a breaking change in 2.0, I would weigh that against how many security holes it would fix, for people who were not aware that this is a special case.

The behavior has been left alone and The User Guide has been updated to clarify expectations.