Skip to content

Require binary format in binary importer #5648

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.

Sign up for GitHub

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

Merged
NinoFloris merged 2 commits into from
Mar 28, 2024
Merged

Require binary format in binary importer #5648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e564975
Require binary format for importer
NinoFloris Mar 27, 2024
7100049
Rework format selection code
NinoFloris Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 27 additions & 28 deletions src/Npgsql/Internal/PgTypeInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,24 +114,27 @@ internal PgConverterResolution GetResolution()
return new(Converter, PgTypeId.GetValueOrDefault());
}

bool CachedCanConvert(DataFormat format, out BufferRequirements bufferRequirements)
bool CanConvert(PgConverter converter, DataFormat format, out BufferRequirements bufferRequirements)
{
if (format is DataFormat.Binary)
if (HasCachedInfo(converter))
{
bufferRequirements = _binaryBufferRequirements;
return _canBinaryConvert;
switch (format)
{
case DataFormat.Binary:
bufferRequirements = _binaryBufferRequirements;
return _canBinaryConvert;
case DataFormat.Text:
bufferRequirements = _textBufferRequirements;
return _canTextConvert;
}
Comment thread
NinoFloris marked this conversation as resolved.
}

bufferRequirements = _textBufferRequirements;
return _canTextConvert;
return converter.CanConvert(format, out bufferRequirements);
}

public BufferRequirements? GetBufferRequirements(PgConverter converter, DataFormat format)
{
var success = HasCachedInfo(converter)
? CachedCanConvert(format, out var bufferRequirements)
: converter.CanConvert(format, out bufferRequirements);

var success = CanConvert(converter, format, out var bufferRequirements);
return success ? bufferRequirements : null;
}

Expand All @@ -141,7 +144,7 @@ internal bool TryBind(Field field, DataFormat format, out PgConverterInfo info)
switch (this)
{
case { IsResolverInfo: false }:
if (!CachedCanConvert(format, out var bufferRequirements))
if (!CanConvert(Converter, format, out var bufferRequirements))
{
info = default;
return false;
Expand All @@ -150,9 +153,7 @@ internal bool TryBind(Field field, DataFormat format, out PgConverterInfo info)
return true;
case PgResolverTypeInfo resolverInfo:
var resolution = resolverInfo.GetResolution(field);
if (HasCachedInfo(resolution.Converter)
? !CachedCanConvert(format, out bufferRequirements)
: !resolution.Converter.CanConvert(format, out bufferRequirements))
if (!CanConvert(resolution.Converter, format, out bufferRequirements))
{
info = default;
return false;
Expand Down Expand Up @@ -217,27 +218,25 @@ internal PgConverterInfo Bind(Field field, DataFormat format)
return new(this, converter, bufferRequirements.Write);
}

// If we don't have a converter stored we must ask the retrieved one.
DataFormat ResolveFormat(PgConverter converter, out BufferRequirements bufferRequirements, DataFormat? formatPreference = null)
{
// First try to check for preferred support.
switch (formatPreference)
{
// The common case, no preference means we default to binary if supported.
case null or DataFormat.Binary when HasCachedInfo(converter) ? CachedCanConvert(DataFormat.Binary, out bufferRequirements) : converter.CanConvert(DataFormat.Binary, out bufferRequirements):
case DataFormat.Binary when CanConvert(converter, DataFormat.Binary, out bufferRequirements):
return DataFormat.Binary;
// In this case we either prefer text or we have no preference and our converter doesn't support binary.
case null or DataFormat.Text:
var canTextConvert = HasCachedInfo(converter) ? CachedCanConvert(DataFormat.Text, out bufferRequirements) : converter.CanConvert(DataFormat.Text, out bufferRequirements);
if (!canTextConvert)
{
if (formatPreference is null)
throw new InvalidOperationException("Converter doesn't support any data format.");
// Rerun without preference.
return ResolveFormat(converter, out bufferRequirements);
}
case DataFormat.Text when CanConvert(converter, DataFormat.Text, out bufferRequirements):
return DataFormat.Text;
default:
throw new ArgumentOutOfRangeException();
// The common case, no preference given (or no match) means we default to binary if supported.
if (CanConvert(converter, DataFormat.Binary, out bufferRequirements))
return DataFormat.Binary;
if (CanConvert(converter, DataFormat.Text, out bufferRequirements))
return DataFormat.Text;

ThrowHelper.ThrowInvalidOperationException("Converter doesn't support any data format.");
bufferRequirements = default;
return default;
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Npgsql/NpgsqlBinaryImporter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ async Task Core(bool async, T value, NpgsqlDbType? npgsqlDbType, string? dataTyp
if (newParam)
_params[_column] = param;

param.Bind(out _, out _);
param.Bind(out _, out _, requiredFormat: DataFormat.Binary);

try
{
Expand Down
22 changes: 11 additions & 11 deletions src/Npgsql/NpgsqlParameter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,7 @@ private protected virtual PgConverterResolution ResolveConverter(PgTypeInfo type
}

/// Bind the current value to the type info, truncate (if applicable), take its size, and do any final validation before writing.
internal void Bind(out DataFormat format, out Size size)
internal void Bind(out DataFormat format, out Size size, DataFormat? requiredFormat = null)
{
if (TypeInfo is null)
ThrowHelper.ThrowInvalidOperationException($"Missing type info, {nameof(ResolveTypeInfo)} needs to be called before {nameof(Bind)}.");
Expand All @@ -622,19 +622,18 @@ internal void Bind(out DataFormat format, out Size size)
ThrowHelper.ThrowNotSupportedException($"Cannot write values for parameters of type '{TypeInfo.Type}' and postgres type '{TypeInfo.Options.DatabaseInfo.GetDataTypeName(PgTypeId).DisplayName}'.");

// We might call this twice, once during validation and once during WriteBind, only compute things once.
if (WriteSize is not null)
if (WriteSize is null)
{
format = Format;
size = WriteSize.Value;
return;
}
if (_size > 0)
HandleSizeTruncation();

if (_size > 0)
HandleSizeTruncation();
BindCore(requiredFormat);
}

BindCore();
format = Format;
size = WriteSize!.Value;
if (requiredFormat is not null && format != requiredFormat)
ThrowHelper.ThrowNotSupportedException($"Parameter '{ParameterName}' must be written in {requiredFormat} format, but does not support this format.");

// Handle Size truncate behavior for a predetermined set of types and pg types.
// Doesn't matter if we 'box' Value, all supported types are reference types.
Expand Down Expand Up @@ -674,7 +673,7 @@ void HandleSizeTruncation()
}
}

private protected virtual void BindCore(bool allowNullReference = false)
private protected virtual void BindCore(DataFormat? formatPreference, bool allowNullReference = false)
{
// Pull from Value so we also support object typed generic params.
var value = Value;
Expand All @@ -684,7 +683,7 @@ private protected virtual void BindCore(bool allowNullReference = false)
if (_useSubStream && value is not null)
value = _subStream = new SubReadStream((Stream)value, _size);

if (TypeInfo!.BindObject(Converter!, value, out var size, out _writeState, out var dataFormat) is { } info)
if (TypeInfo!.BindObject(Converter!, value, out var size, out _writeState, out var dataFormat, formatPreference) is { } info)
{
WriteSize = size;
_bufferRequirement = info.BufferRequirement;
Expand All @@ -694,6 +693,7 @@ private protected virtual void BindCore(bool allowNullReference = false)
WriteSize = -1;
_bufferRequirement = default;
}

Format = dataFormat;
}

Expand Down
7 changes: 4 additions & 3 deletions src/Npgsql/NpgsqlParameter`.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,17 +91,17 @@ private protected override PgConverterResolution ResolveConverter(PgTypeInfo typ
}

// We ignore allowNullReference, it's just there to control the base implementation.
private protected override void BindCore(bool allowNullReference = false)
private protected override void BindCore(DataFormat? formatPreference, bool allowNullReference = false)
{
if (_asObject)
{
// If we're object typed we should not support null.
base.BindCore(typeof(T) != typeof(object));
base.BindCore(formatPreference, typeof(T) != typeof(object));
return;
}

var value = TypedValue;
if (TypeInfo!.Bind(Converter!.UnsafeDowncast<T>(), value, out var size, out _writeState, out var dataFormat) is { } info)
if (TypeInfo!.Bind(Converter!.UnsafeDowncast<T>(), value, out var size, out _writeState, out var dataFormat, formatPreference) is { } info)
{
WriteSize = size;
_bufferRequirement = info.BufferRequirement;
Expand All @@ -111,6 +111,7 @@ private protected override void BindCore(bool allowNullReference = false)
WriteSize = -1;
_bufferRequirement = default;
}

Format = dataFormat;
}

Expand Down