ISSUE#31 Fixed panic on nil ptr

[lombare]

Pull request related to #31

[mweibel]

thanks!

[simaotwx]

I think the slice case should be removed because a nil slice is effectively an empty slice.
This caused a regression in our codebase. Instead of [] being in the response nownull is which causes an error in the frontend.

[mweibel]

hmm, you're right. same for the map actually.
would you mind creating a PR fixing this?

[simaotwx]

Sure, I'll do that right away.

[simaotwx]

While writing tests, I'm getting a fail on the map part.
On v0.10.0 maps were already being marshaled as null.

Here is the test result:

--- FAIL: TestMarshal_NilMap (0.00s)
    sheriff_test.go:762: 
        	Error Trace:	sheriff_test.go:762
        	Error:      	Not equal: 
        	            	expected: "{}"
        	            	actual  : "null"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-{}
        	            	+null
        	Test:       	TestMarshal_NilMap
FAIL
exit status 1
FAIL	github.com/liip/sheriff	0.005s

There is a test that expects null for maps:

https://github.com/liip/sheriff/blob/master/sheriff_test.go#L493

[simaotwx]

If keeping existing behavior is desired, I would suggest only removing reflect.Slice from the switch. This way the tests still work and the new one testing nil slices does, too.

[simaotwx]

The interesting thing about this is that json.Marshal also marshals a nil slice to null so it seems like this library has always deviated from that behavior. In case this is supposed to mimic how json.Marshal would handle this case, the current implementation is correct, but breaks existing codebases that falsely relied on this behavior (although since semver allows breaking changes on 0.x I don't see a big issue here).

[simaotwx]

In codebases using this library, this can be fixed/worked around by changing

var slice []T

to

slice := []T{}

[mweibel]

hmm not sure what to do here TBH.

var vs := does have a slight difference (var does not allocate memory IIRC) but otherwise that should be ok.

[mweibel]

The initial issue was only for nil pointer and then the PR extended into other types too.
Let's merge your PR, but fix the failing test first?

[simaotwx]

var vs := does have a slight difference (var does not allocate memory IIRC) but otherwise that should be ok.

The difference between the two is that slice := []T{} creates an empty slice whereas var slice []T creates a nil slice (so there's no underlying storage for the slice and the value is nil).

The initial issue was only for nil pointer and then the PR extended into other types too. Let's merge your PR, but fix the failing test first?

The failing test is not fixable on its own, so I created another branch with only reflect.Slice removed which only requires one test and it doesn't fail in that case: simaotwx@0b23eee

Should I open a PR for that one?

[mweibel]

yeah, let's do that.