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

add functionality to pass spanner types to insert/update #154

Conversation

matthewjumpsoffbuildings
Copy link
Contributor

This is a first attempt to resolve #149

The first step is to add a $types array to both Models and Builders, and to automatically inject a Models types array into every new Builder instance for that model.

The next step is overriding the base update/insert methods of the Builder, to loop through each value being passed in, and using a function checkForType() that attempts to apply the same conventions as the Parameterizer class, to generate an array of bound types eg ['p2' => Database::TYPE, ...].

The final step is adding an additional optional $types = [] argument to Connection update / insert / statement / affectingStatement methods, so that it can be passed to the $transaction->executeUpdate() as an additional types argument alongside parameters

I am still not super familiar with how all the internals work here, likely there are several improvements that could be made, but hopefully this is a good jumping off point

@matthewjumpsoffbuildings
Copy link
Contributor Author

matthewjumpsoffbuildings commented Dec 9, 2023

Theres some issues with the types of some of the methods coming up in the unit tests, hopefully if the general ideas of this PR work, you dont mind sorting those, I put some ugly forced type docs in to get the phpstan stage passing, all of it needs a cleanup

@taka-oyama
Copy link
Collaborator

I was looking through the underlying implementation and it looks like it would be easier if you implement a Json class which implements ValueInterface and pass that as binging.

Coversion takes place here.

@matthewjumpsoffbuildings
Copy link
Contributor Author

Right so perhaps I could create a custom Cast class that uses the ValueInterface? That would be much simpler, this was a very hacky attempt

@matthewjumpsoffbuildings
Copy link
Contributor Author

Though I notice this in the comments:

* Note: This interface may become internal in the next major version change

Which may be concerning?

@taka-oyama
Copy link
Collaborator

In that case I think it's best to throw a PR to them to add a JSON type so it's handled internally.
Seems like they already have one for PgJsonb.

@matthewjumpsoffbuildings
Copy link
Contributor Author

matthewjumpsoffbuildings commented Dec 11, 2023

Just checking, how do I use these google type interfaces anyway? Am I on the right track to assume its via Casts?

Also I need JSON working yesterday haha, I am not sure how long a PR will take with google

@matthewjumpsoffbuildings
Copy link
Contributor Author

matthewjumpsoffbuildings commented Dec 11, 2023

Are you familiar with the ValueMapper class? Apparently this is a thing:

$jsonValue = new PgJsonb('{"key": "value"}');

$valueMapper = new ValueMapper($db, true);

$mappedValue = $valueMapper->encode($jsonValue);

$result = $db->execute('INSERT INTO MyTable (JsonColumn) VALUES (@jsonValue)', [
   'parameters' => [
       'jsonValue' => $mappedValue
   ]
]);

I feel like somewhere in the internals of this library, this ValueMapping should be done?

@taka-oyama
Copy link
Collaborator

ValueMapper is not accessible from our end.

I've already attempted to make it accessible (googleapis/google-cloud-php#5700).

This was their response.

ValueMapperInterface isn't very easy to implement for a developer as-is. It was meant to be an internal class and not exposed externally. Making it public will mean we will have to support it for back-compat (Google's other language libraries also don't seem to expose it). It also confuses users with a similar name class Spanner/src/ValueInterface.php.

@taka-oyama
Copy link
Collaborator

taka-oyama commented Dec 11, 2023

Just checking, how do I use these google type interfaces anyway? Am I on the right track to assume its via Casts?

I had to do this for Numeric types. You will have to convert to a ValueInterface when inserting/updating by overriding Model::getDirtryForUpdate and Model::getAttributesForInsert.

@matthewjumpsoffbuildings
Copy link
Contributor Author

matthewjumpsoffbuildings commented Dec 11, 2023

Holy moly i figured it out and its even easier - no changes required to your code at all.

If I make a custom cast like so:

namespace App\Casts;

use Illuminate\Contracts\Database\Eloquent\CastsAttributes;
use Google\Cloud\Spanner\PgJsonb;
use Google\Cloud\Spanner\V1\TypeAnnotationCode;

class SpannerJsonCast implements CastsAttributes
{
	public function get($model, $key, $value, $attributes)
	{
		return json_decode($attributes[$key], true);
	}

	public function set($model, $key, $value, $attributes)
	{
		return [$key => new SpannerJsonValue($value)];
	}
}

// Just copy the PgJsonb ValueInterface class
class SpannerJsonValue extends PgJsonb
{
	// override the PgJsonb classes typeAnnotation TypeAnnotationCode::PG_JSONB
	public function typeAnnotation()
	{
		return TypeAnnotationCode::TYPE_ANNOTATION_CODE_UNSPECIFIED;
	}
}

Then in my model I use that cast like so:

	protected $casts = [
		'ColumnName' => SpannerJsonCast::class,
	];

Then it just works! This should be added to the docs, it solves any possible type issues on insert/update, not just JSON

@matthewjumpsoffbuildings
Copy link
Contributor Author

You said

I had to do this for Numeric types.

I am noticing that if I have an INT64 column, then try to update/insert to it via an Eloquent model, even with a cast to integer, it throws an error from spanner saying it cant write strings to int columns... I tried making another custom cast class and that solved it, I am wondering where the implementation for your Numeric type solution is? Or has it not been released?

@taka-oyama
Copy link
Collaborator

taka-oyama commented Dec 12, 2023

it throws an error from spanner saying it cant write strings to int columns

Doesn't happen to me.
If you pass a form input, which is a string, this will happen.
You have to cast it to an int first like $model->number = (int) $value.

I am wondering where the implementation for your Numeric type solution is? Or has it not been released?

This is not released to public because it's not a very clean solution.

@matthewjumpsoffbuildings
Copy link
Contributor Author

Doesn't happen to me. If you pass a form input, which is a string, this will happen. You have to cast it to an int first like $model->number = (int) $value.

I am using an Eloquent model, with $casts = [ 'column' => 'int'], and the form is actually a Laravel Nova form, using a Number field. None of this works, always seem to get the 'cannot insert string into int64' error from spanner. The only thing that did work was making a custom cast class that uses an extension of the Numeric value type

@taka-oyama
Copy link
Collaborator

taka-oyama commented Dec 12, 2023

Then override Model::getDirtryForUpdate and Model::getAttributesForInsert and cast it to an int?

@taka-oyama
Copy link
Collaborator

taka-oyama commented Dec 12, 2023

Also, please provide as much context as possible so we don't have to go back and forth.

@taka-oyama
Copy link
Collaborator

Custom type casting will be improved once google implements googleapis/google-cloud-php#5838. Closing this for now.

@taka-oyama taka-oyama closed this Feb 20, 2024
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.

JSON columns dont appear to be working
2 participants