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

Draft: fix JSONType[T] Pluck query error #201

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

alingse
Copy link
Contributor

@alingse alingse commented Apr 7, 2023

  • Do only one thing
  • Non breaking API changes
  • Tested

What did this pull request do?

This is a fix MR for #200

and this cause a little breaking change in JSONType[T] usage

the bug

I ever post PR #174 , but I didn't have test enough for ALL gorm query API.
this exported Data T field worked well in Find, First method, and added in json_type_test.go.
and Data() T method is not so efficient than directly exported field , but it is wrong.

This issue #200 give a bad case query in Pluck

Gorm found the input ptr of value var field = &JSONType[*User] is a struct , and have an exported Field Data T
so it goto the this Struct's schema.Fields logic, and found this struct's Data T and checked field.DataType == ""

https://github.com/go-gorm/gorm/blob/532e9cf4ccce927249bcb102c09e4a9093aae4fe/schema/schema.go#L280-L283

so finally got an error

[error] invalid field found for struct gorm.io/datatypes.JSONType[*main.User]'s field Data: define a valid foreign key for relations or implement the Valuer/Scanner interface
find field in row  <nil> &{name 18}

the fix

I research the gorm and found it only collect ast.IsExported Field to scheme.Fields

and the JSONType[T] provide generic method Valuer and Scaner is only for a field not for another relation Table.

so I think make Data unexported is a better fix solution.

what about the breaking change

I this this breaking is small, and easy enough to change

result.Data ----> result.Data()

If same name is not ok. may be change to result.GetData()

Feel sorry to introduce some bug and cause a break change😢

User Case Description

		type Attribute struct {
			Sex   int
			Age   int
			Orgs  map[string]string
			Tags  []string
			Admin bool
			Role  string
		}
		type UserWithJSON struct {
			gorm.Model
			Name       string
			Attributes datatypes.JSONType[Attribute]
		}

		// Pluck
		var attr datatypes.JSONType[Attribute]
		DB.Model(&UserWithJSON{}).Limit(1).Order("id asc").Pluck("attributes", &attr)
		var attribute = attr.Data()
		AssertEqual(t, attribute.Age, 18)

Note for JSONSlice[T]

sadly db.Pluck still failed for JSONSlice[T] dest, because gorm will reflect the dest and process the Array and Slice

		reflectValueType := reflectValue.Type()
		switch reflectValueType.Kind() {
		case reflect.Array, reflect.Slice:
			reflectValueType = reflectValueType.Elem()
		}

so, I add a Note in README.md for JSONSlice[T]

@alingse
Copy link
Contributor Author

alingse commented Apr 7, 2023

cc @jinzhu I will add more test for all query for this JSONType[T] and also JSONSlice[T] later in weekend.

@alingse alingse force-pushed the unexport-json-type branch 2 times, most recently from d2ca14b to 97cb352 Compare April 10, 2023 15:11
add more test for JSONType

disable JSONSlice Pluck

add docs
@alingse alingse force-pushed the unexport-json-type branch from 83a34d6 to 305be2b Compare April 10, 2023 15:31
@alingse
Copy link
Contributor Author

alingse commented Apr 10, 2023

@jinzhu more test about query added for JSONType[T] and JSONSlice[T], the pr fix JSONType[T] Pluck query error (with an breaking API changes), but JSONSlice[T] Pluck query error still exists which might not be solved.

@alingse alingse changed the title fix JSONType[T] Pluck query error Draft: fix JSONType[T] Pluck query error Apr 11, 2023
@jinzhu jinzhu merged commit 88d502b into go-gorm:master Apr 11, 2023
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.

2 participants