A documented field beneath an optional field with a null value is incorrectly considered to be missing

[tomaszalusky]

Hi,

I ran into issue which can be best summarized by following test method in org.springframework.restdocs.payload.JsonContentHandlerTests class.
I would like test to pass but test fails since a.b is considered missing unless FieldDescriptor a.b is explicitly marked as .optional().

	@Test
	public void describedMissingFieldThatIsChildOfOptionalObjectThatIsNullIsNotConsideredMissing() {
		List<FieldDescriptor> missingFields = new JsonContentHandler(
				"{\"a\":null}".getBytes()).findMissingFields(
						Arrays.asList(new FieldDescriptor("a").optional(),
								new FieldDescriptor("a.b")));
		//System.out.println(missingFields.get(0).getPath());
		assertThat(missingFields.size()).isEqualTo(0);
	}

I consider this approach inconsistent with describedMissingFieldThatIsChildOfNestedOptionalArrayThatIsEmptyIsNotConsideredMissing test where thanks to parent being optional, absence of c field is no problem although it is not optional itself. Why the b field in my example can't be treat the same way?

I appreciate that the support of inheriting optional flag was added in 1.2.5/2.0.2. It is great feature since it makes possible to control optionality only for root of potentially complex tree. However it seems to me as related only to arrays rather than objects. Am I missing something? Thanks for response.

(Note: The test passes on empty json {}. I intentionally don't reveal my investigation of source code, at first I'm interested in what should be correct at conceptual level.)

(Note: IMHO the test case is worth adding no matter what is correct behavior from your point of view.)

[wilkinsona]

Thanks for the detailed report. I think your test should pass too. I suspect that it's not due to a explicitly being null rather than simply being absent. A field that's present but with a null value should be treated in the same way as a field with an empty collection value or that's not there at all.

[tomaszalusky]

Weird... are we talking about same revision?

I work on cloned spring-restdocs repo with updated master (see below) and test fails.
The unhappy path is: findMissingFields -> isNestedBeneathMissingOptionalField -> isMissing -> isEmptyCollection where isMissing is called for parent FieldDescriptor and isEmptyCollection receives null.

Probably the isMissing method should prevent reaching isEmptyCollection method in code flow - either by special if or modifying fieldProcessor.hasField behavior (since the tree traversing looks like fixed algorithm honoring template design pattern, I'm not sure of impact to other code parts).

tz@tomaszalusky MINGW64 /c/p/spring-restdocs (master)
$ git status
On branch master
Your branch is up-to-date with 'origin/master'.
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

        modified:   spring-restdocs-core/src/test/java/org/springframework/restdocs/payload/JsonContentHandlerTests.java

Untracked files:
  (use "git add <file>..." to include in what will be committed)

        spring-restdocs-core/out/

no changes added to commit (use "git add" and/or "git commit -a")

tz@tomaszalusky MINGW64 /c/p/spring-restdocs (master)
$ git log -1
commit 24c1746b363d08a0d369565134ef338cd8069350
Merge: 2b01f67 eb03213
Author: Andy Wilkinson <awilkinson@pivotal.io>
Date:   Thu Oct 4 12:26:50 2018 +0100

    Merge branch '1.2.x'

tz@tomaszalusky MINGW64 /c/p/spring-restdocs (master)
$ git pull
Already up-to-date.

[wilkinsona]

Sorry for the confusion. I am agreeing with you that the current behaviour is wrong and that your test should pass but does not. Hence the label and milestone assigned to this issue.