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

Wip update to Npgsql 4.0 #1045

Closed
wants to merge 21 commits into from
Closed

Wip update to Npgsql 4.0 #1045

wants to merge 21 commits into from

Conversation

oskardudycz
Copy link
Collaborator

@oskardudycz oskardudycz commented Jul 14, 2018

@@ -27,9 +31,10 @@
<EmbeddedResource Include="Schema\SchemaObjects.sql" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="Newtonsoft.Json" Version="9.0.1" />
<PackageReference Include="Newtonsoft.Json" Version="11.0.2" />
<PackageReference Include="Npgsql" Version="4.1.0-ci.1131" />
Copy link
Contributor

Choose a reason for hiding this comment

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

now when npgsql https://www.nuget.org/packages/Npgsql/4.0.2 with jsonb fix is released is this pre-release package necessary ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think so, but I'll double check that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jacobpovar You're right 4.0.2 is enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

great :)

@@ -161,7 +154,17 @@ private string buildSubQuery(SubQueryExpression subQuery, CommandBuilder command
{
throwNotSupportedContains();
}
var fromParam = command.AddParameter(from.Value);

var values = new List<string>();
Copy link
Member

Choose a reason for hiding this comment

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

Why are we having to do this? Is this something that got broken by npgsql 4?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems so, we're passing the parameter as array of text (see here ). I believe that previously it was the automated conversion of parameter, but right now it's not being performed. It throws exception that TextHandler cannot handle array of guids (or something like that). In general it's because Array of String != Array of Guids.

public void ConfigureCommand(CommandBuilder builder)
{
var sql = _template.CommandText;
for (var i = 0; i < _setters.Length && i < _template.Parameters.Count; i++)
{
var param = _setters[i].AddParameter(_model, builder);

if (param.Value is Enum)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this here? Marten has configurable enum storage of either "as strings" or "as int". This would possibly break here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, it's ugly hack. Without this casting it will throw that it doesn't know the Enum type. I need to revisit that but I believe that here it's only for querying and there would be some automated translation. I need to check it again if there is a chance to do it better.

@@ -121,6 +121,8 @@ private void load(ITenant tenant, ISerializer serializer, NpgsqlConnection conn,
_transferData(document, _mapping.AliasFor(document.GetType()), serializer, writer, textWriter, tenant.TenantId);
textWriter.Clear();
}

writer.Complete();
Copy link
Member

Choose a reason for hiding this comment

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

This is new in npgsql4?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe so. @jacobpovar ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct.

From npgsql 4 breaking changes:

You must now call NpgsqlBinaryImporter.Complete() to save your imported data; not doing so will roll the operation back.

@@ -38,9 +37,19 @@ public DuplicatedField(EnumStorage enumStorage, MemberInfo[] memberPath) : base(
PgType = "varchar";
}
else if (MemberType.IsDateTime())
{
Copy link
Member

Choose a reason for hiding this comment

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

Are all the Datetime/DatetimeOffset query tests passing?

Copy link
Collaborator Author

@oskardudycz oskardudycz Jul 17, 2018

Choose a reason for hiding this comment

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

Yes. They changed a lot in the DateTime handling (see links that I attached in the PR description). Without that and other changes it won't work.

@@ -90,6 +88,9 @@ public SprocCall JsonBodies(string argName, ArraySegment<char>[] bodies)

public SprocCall Param(string argName, object value, NpgsqlDbType dbType)
{
if ((value is Enum || value is Guid) && dbType == NpgsqlDbType.Varchar)
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here about the enum configuration on the serializer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here it's the same case as I described with Array of Guids. It seems that there is no more automatic conversion here. They provided some changes to automatically selecting the handlers, but I don't know precisely, because it's a lack of documentation.

@jeremydmiller jeremydmiller added this to the 3.0 Alpha 1 milestone Jul 19, 2018
jeremydmiller added a commit that referenced this pull request Jul 19, 2018
@jeremydmiller
Copy link
Member

Hey everyone, this is now in the official "3.0" branch.

@oskardudycz
Copy link
Collaborator Author

🎉 🎉 🎉

@jacobpovar
Copy link
Contributor

awesome!

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.

4 participants