[GODRIVER-1340] Marshaller interface usage Created: 11/Oct/19  Updated: 05/Dec/19  Resolved: 25/Nov/19

Status: Closed
Project: Go Driver
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: Thomas Bruyelle Assignee: Divjot Arora (Inactive)
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Description   

If I use the `Marshaller` interface on a type to output a slice, it fails with the following error :

`WriteArray can only write a Array while positioned on a Element or Value but is positioned on a TopLevel`

You can see this in https://play.golang.org/p/Sc5Fk_ACvpB

From what I investigated I understood that the call to `bson.Marshal` I do inside `MarshalBSON` thinks the slice is at top level and so refuse to marshal it. Is there an other way to marshal bson w/o that constraint ?



 Comments   
Comment by Thomas Bruyelle [ 05/Dec/19 ]

Hi Divjot, thanks this is exactly what I expected to do for the implementation. I wait for the v1.2.0 release now.

Have a good day

Comment by Divjot Arora (Inactive) [ 05/Dec/19 ]

Hi thomas.bruyelle@keeneye.tech,

Just wanted to let you know that GODRIVER-1416 has been merged. I prototyped a quick Gist using your structs and the new function to ensure that this solves your issue: https://gist.github.com/divjotarora/60724214804dfae6a5aa48b1c0657f95. Also, I verified that our RawValue.Unmarshal does return an error if you try to unmarshal a [][][]float64 into the []float64 field so the extra length check that you have in SetBSON isn't necessary when using UnmarshalBSONValue. As always, let us know if you have any other questions!

 

– Divjot

Comment by Divjot Arora (Inactive) [ 25/Nov/19 ]

thomas.bruyelle@keeneye.tech Glad to hear it worked! I definitely agree that a MarshalValue function would be a much cleaner abstraction and would remove a lot of the complexity. We will keep this in mind when determining the priority of that ticket. Sorry this took so long and thank you for your patience while we figured all of this out. I'm going to resolve this ticket, but feel free to comment on it again or open another ticket if you have any other questions.

 

– Divjot

Comment by Thomas Bruyelle [ 25/Nov/19 ]

Hi Divjot

Thanks it works perfectly well ! I used your gist and adapted it to my Vertices type.

Thanks a lot for your time, I think having this bson.MarshalValue function would be helpful because the implementation is a little tricky .

Comment by Divjot Arora (Inactive) [ 22/Nov/19 ]

Hi thomas.bruyelle@keeneye.tech,

I think I've found a way to make MarshalBSONValue work for you. Unfortunately, our API currently does not have a method that goes from a Go value to a generic BSON value (something like bson.Marshal but it could output any BSON value, not just documents). Because of this, the easiest way to work around is it to marshal a temporary struct and then extract the fields you need from the outputted BSON. I've filed GODRIVER-1416 to add a generic bson.MarshalValue function that would make this much easier to do. The gist I linked in that ticket has an example of how MarshalBSONValue would be implemented right now for a use case like yours and how it could be implemented once the ticket is done. Your implementation of UnmarshalBSONValue should be pretty much the same as the one in the gist.

Can you try porting over that gist to your use case and let me know if that works? I understand this isn't the most elegant way to do it, but it should serve as a workaround for you until we implement a bson.MarshalValue.

Comment by Divjot Arora (Inactive) [ 21/Nov/19 ]

You're right that MarshalBSONValue is probably the way to go. I had some trouble converting the GeoJSON field to BSON when I tried to prototype this, but I'm going to try to prioritize this ticket so I can get you an answer in the next few days. Thank you for being so patient while we figure all of this out.

Comment by Thomas Bruyelle [ 20/Nov/19 ]

Hi Divjot,

Thx for your response. Implementing MarshalBSON in the outer struct isn't acceptable because it's quite large and I don't want to iterate over all its fields. MarshalBSONValue sounds promising, but its usage isn't straightforward from what I tried. You need to return bson in []byte format but the only method I know for that purpose is bson.Marshal and it doesn't accept arrays. If you could me find the correct method to do that I would appreciate. Thx

Comment by Divjot Arora (Inactive) [ 20/Nov/19 ]

Hi thomas.bruyelle@keeneye.tech,

Sorry, I did not see your previous message. MarshalBSON definitely wants a document returned, but the resulting document in the annotations collection looks like that because the Vertices struct is a field of another struct, so the document gets nested. In this case, you can either implement MarshalBSON on the outer struct instead of Vertices or implement MarshalBSONValue on Vertices instead of MarshalBSON. I don't have much experience using MarshalBSONValue, but it should return just the value without the name. Let us know if you need any help implementing either of these solutions.

Comment by Thomas Bruyelle [ 20/Nov/19 ]

Sorry Divjot did you read my previous message ?

Comment by Thomas Bruyelle [ 13/Nov/19 ]

Hi Divjot sorry I made further tests and I can't confirm the behavior we previously stated.

If I define a MarshalBSON that returns the exact document, and then query that document in the mongo console, I get an extra undesired layer !

type VerticesGeneric struct {
    Vertices interface{}
}
 
func (v Vertices) MarshalBSON() ([]byte, error) {
    if len(v.GeoJSON) > 0 {
        return bson.Marshal(VerticesGeneric{v.GeoJSON})
    }
    return bson.Marshal(VerticesGeneric{v.Float64s})
}

> db.annotations.find()
{ "_id" : "5dcbc117cc1ee077a0d8e1a8",  "vertices" : { "vertices" : [ 1, 1, 2, 2, 3, 3, 4, 4 ] }, "location" : [ -162.421875, -85.60546875 ] }

I don't want to have a field "vertices" : {"vertices" : [1,1,2,2,3,3,4,4]}, what I really want is "vertices": [1,1,2,2,3,3,4,4]

So from what I see, MarshalBSON want us to return the value of the field, just like GetBSON...

Comment by Thomas Bruyelle [ 12/Nov/19 ]

Thanks for your help and your time Divjot, I have all that I need to move forward.

Comment by Divjot Arora (Inactive) [ 12/Nov/19 ]

I can definitely see why that's confusing. As I mentioned above, the semantics for MarshalBSON and GetBSON are different:

1. MarshalBSON should be implemented for struct types and expects the return value to be a document. This is the exact document that will get sent to the server for something like InsertOne or InsertMany.

2. GetBSON expects that you will return an interface{} that mgo can then marshal. This does not necessarily have to be a document as long as it fits into a document later on.

We are currently working on a project to add mgo-compatible BSON codecs to make migration from mgo easier. Part of this project is to add support for the Getter and Setter interfaces (GODRIVER-1356).

Sorry for the misunderstanding on my end for this ticket. Hopefully you have enough to have a path forward for migration. I'm going to close this ticket as resolved, but feel free to re-open or open another ticket if you have any questions.

Comment by Thomas Bruyelle [ 12/Nov/19 ]

Oh ok that's why this couldn't work, I need to fix my code. 

I just need to know something, my previous implementation of mgo.bson.Getter used to return the value of the field, not the document. And the final result was the expecting one. Does this mean mgo.bson.Getter and bson.Marshaller doesn't have the same behavior, the first one requires the value of the field while the second one requires the whole document (field name + value) ?

As a remind, this was my implementation of mgo.bson.Getter, as you can see, I return only the value of the field.

// GetBSON implements the bson.Getter interface to select the correct vertice
// format.
func (v Vertices) GetBSON() (interface{}, error) { 
  if len(v.GeoJSON) > 0 { 
    return v.GeoJSON, nil 
  } 
  return v.Float64s, nil
}

Comment by Divjot Arora (Inactive) [ 12/Nov/19 ]

That makes more sense. MarshalBSON should output a document, not a value. This is because a struct corresponds to a document (and a nested struct field corresponds to an embedded document). So for the Vertices type, MarshalBSON must return something like

{"vertices": [1.0, 2.0, 3.0]}

. If you can give us the type definition of the larger struct type, we can help you implement the correct set of interface methods for it.

Comment by Thomas Bruyelle [ 12/Nov/19 ]

Ok, vertices is just a field inside a bigger document. Initially it was only a []float64, but after we had the need to support the GeoJSON format which is [][][]float64. For retro compatibility we decided to implement bson.Marshaller and bson.Unmarshaller, because we wanted to handle the two formats.

Just to make sure we are on the same line, when you implement a bson.Marshaller or bson.Unmarshaller, you deal with the value of the field, not the field name and its value correct ?

Comment by Divjot Arora (Inactive) [ 12/Nov/19 ]

That seems really strange to me. I wrote up a small repro script that tries to insert an instance of Vertices using mgo and got an error: " BSON field 'insert.documents.0' is the wrong type 'array', expected type 'object'". This makes sense given the output you're showing because mgo is outputting an array but MongoDB only accepts documents (there is a small difference: an array is technically a document where the keys are integers, but the BSON value subtype is different so the server errors on insert).

Can you explain why you need the output to be a BSON array rather than a BSON document? Also, I was assuming this is all happening through an mgo Insert call. Is this not true? It doesn't seem like the type can actually be inserted through mgo.

Comment by Thomas Bruyelle [ 12/Nov/19 ]

Hi Divjot, I took the []byte value from the mgo version (with a field like that "vertices":[1,2,3,4]) and prints it with the official driver bson.Raw :

 

b := bson.Raw{49, 0, 0, 0, 1, 48, 0, 0, 0, 0, 0, 0, 0, 240, 63, 1, 49, 0, 0, 0, 0, 0, 0, 0, 0, 64, 1, 50, 0, 0, 0, 0, 0, 0, 0, 8, 64, 1, 51, 0, 0, 0, 0, 0, 0, 0, 16, 64, 0}
fmt.Println("RAW", b)

 

it prints :

RAW {"0": {"$numberDouble":"1.0"},"1": {"$numberDouble":"2.0"},"2": {"$numberDouble":"3.0"},"3": {"$numberDouble":"4.0"}}

Comment by Divjot Arora (Inactive) [ 08/Nov/19 ]

Hi thomas.bruyelle@keeneye.tech, I'm not sure I understand. A bson.D is a document and

bson.D=[

{vertices [1 2 3 4]}

] means that the document being sent to the server looks like 

{vertices: [1, 2, 3, 4]}

To see this more clearly, you can print out the result of the MarshalBSON function as a bson.Raw, which has a nicer String() output.

I think this is correct given the struct definition in the previous comment. If the struct has a field called Vertices, the document sent to the server should have a key vertices (in the absence of any struct tags that might modify that). Can you explain why you think it isn't correct?

Comment by Thomas Bruyelle [ 08/Nov/19 ]

Hi Divjot, thanks for the try but the resulting value is different than expected

Your example returns a bson.D=[{vertices [1 2 3 4]}] where I expect bson.D=[\{0 1} \{1 2} \{2 3} \{3 4}].

Comment by Divjot Arora (Inactive) [ 07/Nov/19 ]

Hi thomas.bruyelle@keeneye.tech,

I think I have a better understanding of this now. mgo's GetBSON had some different semantics than our MarshalBSON. GetBSON allowed you to take a type that couldn't be marshalled and then return a type that could. Our MarshalBSON is a little lower-level and actually gives you full control over the marshalling. It expects that you will return an BSON document and uses that as the result without any extra modifications. As a quick example, something like the following should work:

 

type VerticesGeneric struct {
    Vertices interface{}
}
 
func (v Vertices) MarshalBSON() ([]byte, error) {
    if len(v.GeoJSON) > 0 {
        return bson.Marshal(VerticesGeneric{v.GeoJSON}
    }
    return bson.Marshal(VerticesGeneric{v.Float64s})
}

 

Comment by Thomas Bruyelle [ 07/Nov/19 ]

The source of all my pain is because I can't turn the GetBSON from mgo to

func (v Vertices) MarshalBSON() ([]byte, error) {
  if len(v.GeoJSON) > 0 {
    return bson.Marshal(v.GeoJSON)
  }
  return bson.Marshal(v.Float64s)
}

with the official driver 

 

Comment by Thomas Bruyelle [ 07/Nov/19 ]
  1. is correct
  2. is almost correct, the field is always named vertices, so the document looks like 
    {vertices: [1.0, 2.0, 3.0]}
    OR
    {vertices: [[[1.0, 2.0, 3.0]]]}

3. is correct

This is how we handled the field with mgo :

 

// GetBSON implements the bson.Getter interface to select the correct vertice
// format.
func (v Vertices) GetBSON() (interface{}, error) {
	if len(v.GeoJSON) > 0 {
		return v.GeoJSON, nil
	}
	return v.Float64s, nil
}
 
// SetBSON implements the bson.Setter interface to select the correct vertice
// format.
func (v *Vertices) SetBSON(raw bson.Raw) error {
	if v == nil {
		return nil
	}
	var fs []float64
	err := raw.Unmarshal(&fs)
	// NOTE(tb) I expected raw.Unmarshal returns a bson.TypeError if its content
	// doesn't match []float64, unfortunately it doesn't.
	// So we need to check len(fs) > 0.
	if err == nil && len(fs) > 0 {
		v.Float64s = fs
		return nil
	}
	if _, ok := err.(*bson.TypeError); err != nil && !ok {
		return err
	}
	var fss [][][]float64
	err = raw.Unmarshal(&fss)
	if err == nil && len(fss) > 0 {
		v.GeoJSON = fss
	}
	return err
}

 

 

Comment by Divjot Arora (Inactive) [ 07/Nov/19 ]

Hi thomas.bruyelle@keeneye.tech,

Thanks for the more detailed info. To make sure I'm understanding the issue correctly, can you confirm the following:

1. The struct definition is:

struct Vertices {
    Float64s []float64
    GeoJSON [][][]float64
}

2. Documents in MongoDB that represent this data look like:

{float64s: [1.0, 2.0, 3.0]}
OR
{geojson: [[[1.0, 2.0, 3.0]]]}

3. Either the Float64s field or the GeoJSON field will always be non-empty.

 

Can you also show us some of the mgo code you originally had? That would help us convert it to the driver equivalent.

 

– Divjot

Comment by Thomas Bruyelle [ 07/Nov/19 ]

Thanks for your answer, it helped a little but since you can't inline a slice, it doesn't work me. 

Let me explain my case a little deeper:

In our codebase, for retro-compat reason, we have that struct Vertices that have 2 fields, one is a Float64s []float64 and the other is a GeoJSON [][][]float64. According to what is filled the mongo field will get the value of the first one or the second one.

To implement that behavior, this should be simple as :

// MarshalBSON implements the bson.Marshaller interface to select the correct vertice format.
func (v Vertices) MarshalBSON() ([]byte, error) {
  if len(v.GeoJSON) > 0 {
    return bson.Marshal(v.GeoJSON)
  }
  return bson.Marshal(v.Float64s)
}

This used to work with the mgo driver but not with yours since bson.Marshal() rejects slices with the error WriteArray can only write a Array while positioned on a Element or Value but is positioned on a TopLevel

 

So my problem is really about the support of slice in bson.Marshal (or inline field). 

 

Do you think it's possible to add support of slice in this method, or to support slice in inlined fields ?

Comment by Divjot Arora (Inactive) [ 15/Oct/19 ]

Hi thomas.bruyelle@keeneye.tech,

Thank you for your report. The reason the code throws an error is because we expect structs to map to BSON documents. So when the struct's MarshalBSON function returns the BSON equivalent of a slice, it doesn't match our expectations. The inline struct tag will accomplish what you want, as it is used to move fields embedded in a document up one level. For example,

struct Outer {
    strut Inner {
        Foo int `bson:"foo,inline"
    }
}

will yield the BSON document {Outer: {Foo: <value>}} rather than {Outer: {Inner:

{Foo: <value>}

}} when marshalled.

 

I verified that https://play.golang.org/p/XbVprBQUUmd creates the document you expect. However, we don't currently support inlining pointers to structs and maps as you have in your example. This will be implemented in GODRIVER-1305, which is scheduled to be worked on soon.

Generated at Thu Feb 08 08:36:12 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.