Ensure the node type is set when loading empty lists and objects

[lucko]

Relates to #93, #97

Waiting for a test case before merging. Want to clarify that this does indeed fix the issue reported.

[lucko]

Hmm, the test from this commit is now failing.

bbb93c8

I'm not really convinced that this is testing for the correct behaviour anyways? If an empty object is declared in the JSON, then IMO this should also be represented as an object (map) in configurate.

Would like a second opinion before I modify the test. Any thoughts @parlough @dualspiral ?

[dualspiral]

The test is checking for the reversibility and is checking for the correct behaviour as it stands. At the time, I was simply concerned with an empty document causing things to blow up. If you want to change the behaviour such that {} is a map, then yes, I would change this, but also handle null as a value in a different way. In this case, I was (ab)using the fact that an empty map is null.

Which brings me straight onto my next point, expectations and that we might want to be careful about this. The API isn't just about the code signature, it's about how it functions too, if Java changed how String#replace(String) worked by doing some sort of backwards comparison, it might not be a signature break but you know the whole development community would get somewhat upset!

It's an extreme example, but it's largely the same thought process here. This commit actually changes some of the fundamentals of how the system works. While I have no doubt that what you are proposing is the correct and right behaviour, it's been like this for as long as I remember and there may be developers (again, incorrectly) relying on the behaviour that null is an empty map or list. I suspect that nulls were used for getting values without using a type token, such that type information isn't available to create the correct type of map (though with type erasure, that's probably not so much of a problem either...)

Anyway, my point is that I'm concerned this is going to break Sponge plugins that rely on this behaviour that's been around for ages (null checking rather than empty list checking) - hopefully I'm wrong but any config behaviour that breaks the plugins in a weird way isn't going to be welcomed by the community. Not much of a problem for bleeding, of course, but it could be a bit of a problem for stable.

Maybe add a configuration option for this that defaults to the old behaviour, then afterwards consider making a configurate 4.0 later that removes the bad behaviour and removes some constructs that shouldn't be there.

I would like to see tests that check this new behaviour as well.

[lucko]

The API isn't just about the code signature, it's about how it functions too, if Java changed how String#replace(String) worked by doing some sort of backwards comparison, it might not be a signature break but you know the whole development community would get somewhat upset!

This is a bad example - there's a difference between changing the underlying behaviour of a method for no reason, deviating from the previously documented behaviour, and fixing what is clearly a bug.

In the case of the issue addressed by this PR, and reported in the two issues referenced above, configurate isn't accurately parsing what's actually in the file - perhaps the highest requirement and assumption there could be about the behaviour of the library.

---

More generally, and aside from this PR, I disagree that the "current" behaviour of a program should be considered part of the API contract.

Rather, an API contract is made up of:

• The code signature
• The documented behaviour of the code (JavaDocs for example)
• Expected behaviour (expected vs. current is an important distinction)
  • This is obviously a bit fuzzy, everyone has different interpretations
  • To give an example, you'd expect that even without any documentation, a #clear method would wipe the state of the object

A change made to the implementation to ensure that a method returns in accordance with the documented behaviour, and with what's expected, is not a breaking change in my eyes. It's fixing a bug that meant the method didn't behave in line with its contract previously.

I don't think this relates exactly in this case, but still funny: https://xkcd.com/1172/

[SimonFlash]

I've always been one to prefer explicit behavior - there is a strict difference between defining something as empty versus not defining it at all. The JSON specs are also clear about this in that {} and [] are to be treated as an empty object/list, while something undefined (or defined as null) is indeed null (which is actually considered a value).

At the time, I was simply concerned with an empty document causing things to blow up.

Configurate only supports maps as the root node, so the expected behavior here should be an empty map.

Anyway, my point is that I'm concerned this is going to break Sponge plugins that rely on this behaviour that's been around for ages (null checking rather than empty list checking)

Null checking should (always?) be equivalent to isVirtual in these situations. Developers should know how to handle the difference between empty and undefined and handle them accordingly for their use case.

[dualspiral]

OK, I need to return to my point that I agree this would be correct behaviour. My point was that of consumer expectation.

With regards to my example: yes, it might have been a bad example, but my point is largely the same - the actual behaviour has been around so long that it could be considered expected because that's how it's worked. I never said "current" and that's not what I intended - if that's the case nothing would ever change.

Intended or expected? The expected behaviour now is that an empty collection or map is a null. Intended? Maybe, maybe not, I don't know what zml intended. Expected, perhaps now it is because there is no documentation to say one way or another. My point is expected behaviour is not only what the code states, but how the community has used it if it is ambiguous.

I agree on the signature names meaning something. getValue gets the read value, but I can see why it could be accepted that null indicates a lack of a value, including an empty list. It shouldn't, but at a stretch, it could.

To be honest, I wouldn't fight much if this ended up in any version, as long as a resonable set of tests are written for this. As I stated previously, I think this is the right behaviour, expected or otherwise. However, I am concerned that such a behaviour change will change how plugins or other consumers use the library that do a null check vs an empty list check - if they don't do both and they have different behaviour based on null/empty because this has been here as long as I remember. I just don't want there to be yet another upset due to a change in configurate that people weren't expecting, especially if it's behaviour that changes that they weren't expecting, even if it was ultimately wrong.

[lucko]

I understand, thank you for clarifying.

Leaving the arguments about expectations, intended behaviour etc behind, I do agree that there could be some impact as a result of this change.

From a versioning point of view in configurate - ignoring Sponge for a second, I personally don't think that a bug-fix of this nature requires a major version bump. Semver refers to "backwards compatible bug fixes", but doesn't really differentiate between binary compatibility and underlying, internal behaviour. Libraries like configurate where the whole project is exposed publicly is never going to have backwards compatible bug fixes.

Re: Sponge, would we push "backwards incompatible" (although I think that label is a stretch) fixes to method behaviours in stable Sponge releases? I think so, probably?!

If it's going to affect a small portion of the Sponge developer community - then personally my opinion would be to just let it happen. I think the majority would prefer for the bug to just be fixed.

So er, what's next. I'll leave this here for now and let others give their opinion. It obviously won't be merged until we (and by "we" I mean more than just you and I) are in agreement about how such a change should be introduced. 🙂

And finally a separate comment about version 4.0
I think 4.0 is likely to be our best chance to make structural changes to the library, and I think we should take our time with it. configurate has transitioned from being zmls personal project to one developed by the Sponge time, primarily for use with the SpongeAPI. I've spoken to several people who have gripes about some of the design choices, and figured we'd develop 4.0 separately for a while to give those people a chance to contribute bigger design changes.

[zml2008]

Alright, I think this change should definitely come in for 4.0. This will also ensure consistency with how object mapping currently treats empty objects.