ensureNotIterable in MongoTemplate only checks for array type [DATAMONGO-2044]

[spring-projects-issues]

Bartłomiej Mazur opened DATAMONGO-2044 and commented

Currently code in MongoTemplate only checks if object is an array, but does not check if it is iterable like name is suggesting as it does check if class name of object is inside list that contains class names of iterator, collection and list interfaces - and interface can't be class of object.

That list should be removed and method changed to simple instanceof check. Or just removed.

spring-data-mongodb/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java

Lines 162 to 170 in 9843325

	static {
	Set<String> iterableClasses = new HashSet<>();
	iterableClasses.add(List.class.getName());
	iterableClasses.add(Collection.class.getName());
	iterableClasses.add(Iterator.class.getName());
	ITERABLE_CLASSES = Collections.unmodifiableCollection(iterableClasses);
	}

spring-data-mongodb/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java

Lines 1151 to 1157 in 9843325

	protected void ensureNotIterable(@Nullable Object o) {
	if (null != o) {
	if (o.getClass().isArray() || ITERABLE_CLASSES.contains(o.getClass().getName())) {
	throw new IllegalArgumentException("Cannot use a collection here.");
	}
	}
	}

 

---

Referenced from: pull request #590

1 votes, 2 watchers

[spring-projects-issues]

Bartłomiej Mazur commented

I made PR for this issue: #590

[spring-projects-issues]

Bartłomiej Mazur commented

Now I'm confused as now I see there is a test that ensures that iterable item will pass here, so maybe this code should be just removed?

spring-data-mongodb/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/mapping/MappingTests.java

Lines 497 to 518 in 9843325

	@Test // DATAMONGO-805
	public void shouldMapFieldsOfIterableEntity() {
	template.dropCollection(IterableItem.class);
	template.dropCollection(Container.class);
	Item item = new IterableItem();
	item.value = "bar";
	template.insert(item);
	Container container = new Container("foo");
	container.item = item;
	template.insert(container);
	Query query = new Query(Criteria.where("id").is("foo"));
	Container result = template.findOne(query, Container.class);
	assertThat(result, is(notNullValue()));
	assertThat(result.item, is(notNullValue()));
	assertThat(result.item.value, is("bar"));
	}

But issue itself is not about iterable entities: https://jira.spring.io/browse/DATAMONGO-805

[spring-projects-issues]

Oliver Drotbohm commented

Would you mind taking a step back and elaborating what code you'd like to write and see not working as expected? What the title claims is not true, as we check the list of well known types.

The primary purpose of the method is to catch the case that a collection or array is not handed to the save(…) method. However, sometimes entities implement Iterable which we need to allow. That's why we can't simply reject everything implementing Iterable

[spring-projects-issues]

Bartłomiej Mazur commented

But this method only checks if it is an array because it is impossible to have object of exactly List/Collection/Iterator type - as these are interfaces.

Now I changed this to check for Iterator or Collection - so Iterable type is still allowed. But I wonder if that check is needed then, maybe we can only check for the array?

[spring-projects-issues]

Oliver Drotbohm commented

Please, again, let's take a step back. What is your use case? What kind of code would you like to execute? Ideally prepare a test case that's failing and actually should not

[spring-projects-issues]

Bartłomiej Mazur commented

I just noticed that bug - useless code that does nothing and does not check anything - while I was looking for something else, nothing more. So I don't have any special use case.

This code looks like it should prevent using lists/collections here, but even simplest new ArrayList or List.of() will not be detected here as this code expects interface type in place where only class type can occur. 

Only arrays will be detected, as it is separate condition in that if statement