Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

decode with inline pointers #217

Merged
merged 7 commits into from
Oct 3, 2018

Conversation

larrycinnabar
Copy link

@larrycinnabar larrycinnabar commented Jul 16, 2018

fix #210

My last PR !146 added ability to encode bson with inline pointers to structs (as json.Marshal does)

Here, we add the ability to decode.
It's a little bit tricky.

help and/or review is appreciated.

  1. only exported inline fields are allowed to be decoded. (as field.CanInterface() will return false for protected inline fields)
    Here couple of tests still needed - to check if proper error message is shown to user (that unexported inline pointers are not allowed)

  2. there are 2 reflect helper created (to create a deep zero element) - that will be the base element when decoded. These functions need to be unit-test covered too.

Current behaviour might seem not correct. What do you think, is it ok?

type MStruct struct {
	M int `bson:"m,omitempty"`
}
type InlinePtrStruct struct {
	A        int
	*MStruct `bson:",inline"`
}

func (s *S) TestPtrInline(c *C) {
		in := InlinePtrStruct{A: 4}
		data, err := bson.Marshal(in)
		c.Assert(err, IsNil)

		out := InlinePtrStruct{}
		err = bson.Unmarshal(data, &out)
		c.Assert(err, IsNil)

		c.Assert(out.A, Equals, 4)
		c.Assert(out.M, Equals, 0)
}

This test passes. But after Unmarshal we get out to be: InlinePtrStruct{A: 4, MStruct: &Mstruct{M:0}}. So instead of nil inline structs we get pointers to empty structs. So, inline is always goes deep, and keeps pointers to empty structs.

@domodwyer
Copy link

Hi @larrycinnabar

Thanks for the PR! Looking good so far.

Regarding the initialised Mstruct instance instead of nil, I think it would be preferred that it was indeed nil as a nil pointer is often used to convey meaning itself and we recently merged some not-totally-definitely-backwards-compatible changes to make it work as expected for slices and maps (thanks @aksentyev!) - do you think it's possible?

Again, thanks very much for take this on!

Dom

@domodwyer domodwyer added the bug label Jul 18, 2018
@larrycinnabar
Copy link
Author

@domodwyer

ok, right, thanks for note.

ckome
ckome previously approved these changes Aug 5, 2018
@larrycinnabar
Copy link
Author

larrycinnabar commented Aug 6, 2018

Fix is pushed. nil-respecting behavior is available through the respect-nil option.

Right before pushing I've found one more case, that should be handled. It's commented in tests. WIll handle it soon.


c.Assert(dataBSON, DeepEquals, cs.Out)
// mixing empty & nil
// @TODO

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just a forgotten test?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think it was a real TODO, not just forgotten test.
I had really turbulent few of weeks too, so I think I need to spend couple hours on this yet.

Let me look into it this weekend and then I'll push / or post here the update status

@domodwyer
Copy link

Hi @larrycinnabar

Really sorry for the delay - it's been a really, really turbulent few of weeks!

I'll review over lunch today.

Dom

@domodwyer
Copy link

Looks great - just the commented test and it's good to go!

@larrycinnabar larrycinnabar changed the title WIP: decode with inline pointers decode with inline pointers Sep 11, 2018
@larrycinnabar
Copy link
Author

I've debugged the issue:

It's expected behaviour in the failing test, as g1 field is omitempty it is lost while marshalling. That's why when we unmarshal back with respect-nil option - we lose it.

Removing omitempty from g1 field from InlineG1 struct makes test pass.
Feel free to remove that test and merge then

@domodwyer
Copy link

Thanks @larrycinnabar!

It's really appreciated :)

Dom

@eminano eminano merged commit 6d406f4 into globalsign:development Oct 3, 2018
@eminano
Copy link

eminano commented Oct 3, 2018

Thanks a lot for your contribution @larrycinnabar and sorry for the delay!

@msolimans
Copy link

@eminano thanks for the merge into dev branch! any plans for release?

@eminano
Copy link

eminano commented Oct 9, 2018

Hi @msolimans, we'll be cutting a release soon, just need to get a bit of time to run all the performance tests required and we're set!

Esther

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants