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

Marshal missing UDT fields as null instead of failing #269

Merged
merged 3 commits into from
Jun 25, 2024

Conversation

sylwiaszunejko
Copy link
Collaborator

We can't return an error in case a field is added to the UDT, otherwise existing code would break by simply altering the UDT in the database. For extra fields at the end of the UDT put nulls to be in line with gocql, but also python-driver and java-driver.

In gocql it was fixed in scylladb/gocql@d2ed1bb

java-driver https://github.com/datastax/java-driver/blob/ef56d561d97adcae48e0e6e8807f334aedc0d783/core/src/main/java/com/datastax/oss/driver/internal/core/type/codec/UdtCodec.java#L86
python-driver: https://github.com/datastax/python-driver/blob/15d715f4e686032b02ce785eca1d176d2b25e32b/cassandra/cqltypes.py#L1036

udt.go Outdated Show resolved Hide resolved
@sylwiaszunejko
Copy link
Collaborator Author

@avelanarius ping

@roydahan roydahan requested a review from dkropachev June 13, 2024 23:45
Copy link
Collaborator

@dkropachev dkropachev left a comment

Choose a reason for hiding this comment

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

We definitely should have test for that

udt.go Outdated
@@ -39,11 +39,17 @@ func makeUDT(value reflect.Value, mapper *reflectx.Mapper, unsafe bool) udt {

func (u udt) MarshalUDT(name string, info gocql.TypeInfo) ([]byte, error) {
value, ok := u.field[name]
if !ok {
return nil, fmt.Errorf("missing name %q in %s", name, u.value.Type())

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we consider unsafe flag ?

func (u udt) MarshalUDT(name string, info gocql.TypeInfo) ([]byte, error) {
	value, ok := u.field[name]
	if ok {
		return gocql.Marshal(info, value.Interface())
	}
	if u.unsafe {
		return nil, nil
	}
	return nil, fmt.Errorf("missing name %q in %s", name, u.value.Type())
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dkropachev AFAIK this would require a user to use query.Iter().Unsafe() instead of just query.Exec(), because in gocql implementation there is no unsafe equivalent. Wouldn't it be counterintuitive if in gocql we are able to do query.Exec() with udt with missing fields and it works, but in gocqlx we are required to use Unsafe()?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sylwiaszunejko , 100% agree on that, but since Unsafe is there and does control this particular behavior we have to be consistent in the way it works.

@sylwiaszunejko
Copy link
Collaborator Author

To sum up after a discussion with @dkropachev:
There is a mechanism in gocqlx for ignoring missing UDT fields, but it requires a user to use query.Iter().Unsafe() instead of just query.Exec(), because in gocql implementation there is no Unsafe equivalent. So on one hand Unsafe is there and does control this particular behavior we have to be consistent in the way it works, but on the other hand we have to be consistent with gocql and if using UDTs with extra fields is supported there always not only when extra option specified, shouldn't it be also supported in gocqlx?
@mykaul do you have any thoughts on that? Because it looks like my fix doesn't take Unsafe mechanism in the consideration and that's probably not the best

@dkropachev
Copy link
Collaborator

To sum up after a discussion with @dkropachev: There is a mechanism in gocqlx for ignoring missing UDT fields, but it requires a user to use query.Iter().Unsafe() instead of just query.Exec(), because in gocql implementation there is no Unsafe equivalent. So on one hand Unsafe is there and does control this particular behavior we have to be consistent in the way it works, but on the other hand we have to be consistent with gocql and if using UDTs with extra fields is supported there always not only when extra option specified, shouldn't it be also supported in gocqlx? @mykaul do you have any thoughts on that? Because it looks like my fix doesn't take Unsafe mechanism in the consideration and that's probably not the best

I would propose to rename this attribute to Strict reversing it's meaning, making default to be false.
Only problem here is that we have to release new major for that, due to the compatibility issue.

@mykaul
Copy link

mykaul commented Jun 14, 2024

I'm in favor of consistency, even if it's the longer path (and complexity). The unsafe is not intuitive.

@dkropachev
Copy link
Collaborator

@mykaul , could you please rereview it, tests were added and more cases fixed.
Also new method was introduced to Queryx to control Unsafe flag on it's level instead of borrowing it from DefaultUnsafe

@mykaul
Copy link

mykaul commented Jun 18, 2024

Do we have an example (in gocqlx docs) for UDT usage?

@sylwiaszunejko
Copy link
Collaborator Author

@mykaul examples are here:

func ExampleUDT() {
and
// This example shows how to add User Defined Type marshalling capabilities to

iterx.go Outdated
Comment on lines 209 to 210
// - ptr to t implements gocql.Unmarshaler, gocql.UDTUnmarshaler or UDT
// - it is not a struct
// - it has no exported fields
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change? Asterisks are valid Markdown itemizers as well AFAIK.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This one is gone, it is already in master, reason why it have changed is golangci-lint does not tollerate anything by - there.

iterx_test.go Show resolved Hide resolved
queryx.go Show resolved Hide resolved
Comment on lines 51 to +59
func (u udt) UnmarshalUDT(name string, info gocql.TypeInfo, data []byte) error {
value, ok := u.field[name]
if !ok && !u.unsafe {
return fmt.Errorf("missing name %q in %s", name, u.value.Type())
if ok {
return gocql.Unmarshal(info, data, value.Addr().Interface())
}

return gocql.Unmarshal(info, data, value.Addr().Interface())
if u.unsafe {
return nil
}
return fmt.Errorf("missing name %q in %s", name, u.value.Type())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure this is the semantics of unsafe that we want to have?
In Rust driver at least, we consider too many and too few fields in the CQL UDT compared to the go struct as two distinct cases. In Rust driver, strict forbids too many fields in CQL UDT, whereas default_if_missing is used for the case when a struct has a field that is not found in the CQL UDT metadata. WDYT? Should we cover it by a single lever here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wprzytula, i am totally in support of having all drivers as close as possible to each other. Let's have separate issue on that and address it in other PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

OTOH I have no idea how other drivers apart from Rust driver approach this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can start from these two

@wprzytula
Copy link
Collaborator

I'm strongly for the change from unsafe to strict, and making non-strict the default. This would be a breaking change, but one that I find justified.

@sylwiaszunejko
Copy link
Collaborator Author

I'm strongly for the change from unsafe to strict, and making non-strict the default. This would be a breaking change, but one that I find justified.

I agree, I would like to go with the strict approach.

@mykaul @avelanarius @roydahan Are we okay with making breaking change in that case?

sylwiaszunejko and others added 2 commits June 19, 2024 08:37
We can't return an error in case a field is added to the UDT,
otherwise existing code would break by simply altering the UDT in the
database. For extra fields at the end of the UDT put nulls to be in
line with gocql, but also python-driver and java-driver.

In gocql it was fixed in scylladb/gocql@d2ed1bb
It enables local control over `unsafe` mode for .Bind methods of `Queryx` and iterators
spawn by it.
…safe mode

We can't return an error in case a field is added to the UDT,
otherwise existing code would break by simply altering the UDT in the
database. For extra fields at the end of the UDT put nulls to be in
line with gocql, but also python-driver and java-driver.

In gocql it was fixed in scylladb/gocql@d2ed1bb
@dkropachev
Copy link
Collaborator

I'm strongly for the change from unsafe to strict, and making non-strict the default. This would be a breaking change, but one that I find justified.

I agree, I would like to go with the strict approach.

@mykaul @avelanarius @roydahan Are we okay with making breaking change in that case?

Let's do that in separate PR.

@roydahan
Copy link
Collaborator

I'm fine with it, we can release a major and state that in the release notes.

@sylwiaszunejko
Copy link
Collaborator Author

sylwiaszunejko commented Jun 25, 2024

Let's merge it and then we can replace unsafe with strict approach in the additional PR. Are you okay with that?
@wprzytula @roydahan @mykaul

@sylwiaszunejko sylwiaszunejko merged commit 6a60650 into scylladb:master Jun 25, 2024
2 checks passed
@mmatczuk
Copy link
Contributor

Interesting.

@mmatczuk
Copy link
Contributor

I'd refrain from bumping the major version and only do that in case of major API breakage i.e. new API, preferably never.

Bumping it to v2 was a complete mess, every import line needs to be changed.
Not that it's hard, but it puts quite a lot of annoying work on customers.

@sylwiaszunejko
Copy link
Collaborator Author

@mmatczuk do you have any suggestion how to handle better the situation with Unsafe and how the default behavior around dealing with missing fields in UDTs differs in gocql and gocqlx?

@mmatczuk
Copy link
Contributor

The change is LGTM.

@sylwiaszunejko
Copy link
Collaborator Author

I am not sure if I understand right, is it okay to do breaking change without major release?

@mmatczuk
Copy link
Contributor

What I'm trying to say is that there is a cost of making /v3 it is not something to do lightly.
Consider what you need, maybe completely removing unsafe and releasing /v3 is a way to go.

@dkropachev
Copy link
Collaborator

dkropachev commented Jun 26, 2024

What I'm trying to say is that there is a cost of making /v3 it is not something to do lightly. Consider what you need, maybe completely removing unsafe and releasing /v3 is a way to go.

Just want to point out that gocqlx recently switched to scylladb/gocql, which also contributes to necessity to bump major up.

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

Successfully merging this pull request may close these issues.

6 participants