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 string overloads to DbDataReader.Get*() #31595

Closed
roji opened this issue Aug 4, 2018 · 81 comments
Closed

Add string overloads to DbDataReader.Get*() #31595

roji opened this issue Aug 4, 2018 · 81 comments

Comments

@roji
Copy link
Member

@roji roji commented Aug 4, 2018

Almost all get methods on DbDataReader require a column index, which is a brittle way to program as the query SQL changes and resultset columns shift around. It's possible to use the pattern reader.GetInt32(reader.GetOrdinal("name")), but that's needlessly verbose. In practice this encourages people to use the reader's indexer (reader["name"]), which is a weakly-typed API returning object (requires casting, boxes value types...).

We can add string overloads to DbDataReader to address this - we would have GetInt32("name"), GetFieldValue<T>("name") etc. Internally these would simply call GetOrdinal(), in effect providing a shortcut for the above syntax, and being 100% backwards-compatible. These would be non-virtual, forcing their behavior to be consistent.

New proposal (via extensions)

After implementation and merge of the string overloads (see original proposal below), EF Core broke since it was looking up DbDataReader methods via reflection, and assuming only one version existed. To remove any chances of similar breakage in other code, it has been proposed to move these methods out as extensions - to be contained in the same assembly (i.e. no new nuget is being proposed).

public static class DbDataReaderExtensions
{        
    public static bool GetBoolean(this DbDataReader reader, string name);
    public static byte GetByte(this DbDataReader reader, string name);
    public static long GetBytes(this DbDataReader reader, string name, long dataOffset, byte[] buffer, int bufferOffset, int length);
    public static char GetChar(this DbDataReader reader, string name);
    public static long GetChars(this DbDataReader reader, string name, long dataOffset, char[] buffer, int bufferOffset, int length);
    public static DbDataReader GetData(this DbDataReader reader, string name);
    public static string GetDataTypeName(this DbDataReader reader, string name);
    public static DateTime GetDateTime(this DbDataReader reader, string name);
    public static decimal GetDecimal(this DbDataReader reader, string name);
    public static double GetDouble(this DbDataReader reader, string name);
    public static Type GetFieldType(this DbDataReader reader, string name);
    public static T GetFieldValue<T>(this DbDataReader reader, string name);
    public static float GetFloat(this DbDataReader reader, string name);
    public static Guid GetGuid(this DbDataReader reader, string name);
    public static short GetInt16(this DbDataReader reader, string name);
    public static int GetInt32(this DbDataReader reader, string name);
    public static long GetInt64(this DbDataReader reader, string name);
    public static Type GetProviderSpecificFieldType(this DbDataReader reader, string name);
    public static object GetProviderSpecificValue(this DbDataReader reader, string name);
    public static Stream GetStream(this DbDataReader reader, string name);
    public static string GetString(this DbDataReader reader, string name);
    public static TextReader GetTextReader(this DbDataReader reader, string name);
    public static object GetValue(this DbDataReader reader, string name);
    public static bool IsDBNull(this DbDataReader reader, string name);

    // For the following see section on async overloads below
    public static Task<T> GetFieldValueAsync<T>(this DbDataReader reader, string name, CancellationToken cancellationToken = default);
    public static Task<bool> IsDBNullAsync(this DbDataReader reader, string name, CancellationToken cancellationToken = default);
}

Async overloads and ValueTask

In the original review, two async string overloads were left out - GetFieldValueAsync() and IsDBNullAsync() - because of the following reasons:

  • It is less commonly used API (only required for sequential access)
  • We were hoping to come up with some new API pattern for handling nullability that takes advantage of the new support for nullable reference types in C#
  • We had doubts on whether new fine-grained async API should return ValueTask instead of Task

After additional discussion we feel that these string overloads should be included. There hasn't been any progress on the story for nullability, and from an API consistency standpoint it would be problematic for all DbDataReader column getters to have string overloads, except for these two. The ordinal-accepting Task-returning methods are already there on DbDataReader, and we'd merely be complementing them with string overloads, as with all the other getters.

We do feel strongly that ValueTask-returning versions are necessary, but consider this to be an orthogonal API addition (tracked by #27682). Even if we had a clear idea of what we want today, it wouldn't be advisable to add a ValueTask-returning GetFieldValueAsync(string) alongside a Task-returning GetFieldValueAsync(int) - we'd likely introduce new method names anyway.

The section replaces #35611.

Original proposal (without extensions)

public partial class DbDataReader
{        
    public bool GetBoolean(string name);
    public byte GetByte(string name);
    public long GetBytes(string name, long dataOffset, byte[] buffer, int bufferOffset, int length);
    public char GetChar(string name);
    public long GetChars(string name, long dataOffset, char[] buffer, int bufferOffset, int length);
    public DbDataReader GetData(string name);
    public string GetDataTypeName(string name);
    public DateTime GetDateTime(string name);
    // Remove from scope - this is a protected virtual method not implemented almost all providers, little interest.
    // public DateTime GetDbDataReader(string name);
    public decimal GetDecimal(string name);
    public double GetDouble(string name);
    public Type GetFieldType(string name);
    public T GetFieldValue<T>(string name);
    // Out of scope for now, pending work on ValueTask-returning overload
    // public Task<T> GetFieldValueAsync<T>(string name); // Forwards to CancellationToken 
    // public Task<T> GetFieldValueAsync<T>(string name, CancellationToken cancellationToken);
    public float GetFloat(string name);
    public Guid GetGuid(string name);
    public short GetInt16(string name);
    public int GetInt32(string name);
    public long GetInt64(string name);
    public Type GetProviderSpecificFieldType(string name);
    public object GetProviderSpecificValue(string name);
    // public int GetProviderSpecificValues(object[] values); <- Inserted by accident, does not belong here
    public Stream GetStream(string name);
    public string GetString(string name);
    public TextReader GetTextReader(string name);
    public object GetValue(string name);
    public bool IsDBNull(string name);
    // Out of scope for now, pending work on ValueTask-returning overload, and possibly a better null-
    // handling strategy
    // public Task<bool> IsDBNullAsync(string name); // Forwards to CancellationToken 
    // public Task<bool> IsDBNullAsync(string name, CancellationToken cancellationToken);
}

Edit history

Date Modification
API shape added
30/1 Updated based on review decisions (removed async overloads)
5/3 New proposal via extension methods, plus add back async overloads
@benaadams

This comment has been minimized.

Copy link
Collaborator

@benaadams benaadams commented Aug 4, 2018

These would be non-virtual, forcing their behavior to be consistent.

Should be virtual so the implementation could cache column lookups etc?

public partial class DbDataReader
{
    public Task<T> GetFieldValueAsync<T>(string name); // Forwards to CancellationToken 
    public virtual Task<T> GetFieldValueAsync<T>(string name, CancellationToken cancellationToken);
    
    public Task<bool> IsDBNullAsync(string name); // Forwards to CancellationToken 
    public virtual Task<bool> IsDBNullAsync(string name, CancellationToken cancellationToken);
    
    public virtual bool GetBoolean(string name);
    public virtual byte GetByte(string name);
    public virtual long GetBytes(string name, long dataOffset, byte[] buffer, int bufferOffset, int length);
    public virtual char GetChar(string name);
    public virtual long GetChars(string name, long dataOffset, char[] buffer, int bufferOffset, int length);
    public virtual DbDataReader GetData(string name);
    public virtual string GetDataTypeName(string name);
    public virtual DateTime GetDateTime(string name);
    public virtual decimal GetDecimal(string name);
    public virtual double GetDouble(string name);
    public virtual Type GetFieldType(string name);
    public virtual T GetFieldValue<T>(string name);
    public virtual float GetFloat(string name);
    public virtual Guid GetGuid(string name);
    public virtual short GetInt16(string name);
    public virtual int GetInt32(string name);
    public virtual long GetInt64(string name);
    public virtual Type GetProviderSpecificFieldType(string name);
    public virtual object GetProviderSpecificValue(string name);
    public virtual int GetProviderSpecificValues(object[] values);
    public virtual DataTable GetSchemaTable();
    public virtual Stream GetStream(string name);
    public virtual string GetString(string name);
    public virtual TextReader GetTextReader(string name);
}
@roji

This comment has been minimized.

Copy link
Member Author

@roji roji commented Aug 4, 2018

Should be virtual so the implementation could cache column lookups etc?

Hmm, shouldn't this be part of the existing GetOrdinal()? It seems that any sort of caching should happen in there...

@benaadams

This comment has been minimized.

Copy link
Collaborator

@benaadams benaadams commented Aug 4, 2018

Better as extensions then?

public static class DbDataReaderExtensions
{
    public static Task<T> GetFieldValueAsync<T>(this DbDataReader reader, string name)
        => reader.GetFieldValueAsync<T>(name, default);
    public static Task<T> GetFieldValueAsync<T>(this DbDataReader reader, string name, CancellationToken cancellationToken)
        => reader.GetFieldValueAsync<T>(reader.GetOrdinal(name), cancellationToken);
    public static Task<bool> IsDBNullAsync(this DbDataReader reader, string name)
        => reader.IsDBNullAsync(reader.GetOrdinal(name), default);
    public static Task<bool> IsDBNullAsync(this DbDataReader reader, string name, CancellationToken cancellationToken)
        => reader.IsDBNullAsync(reader.GetOrdinal(name), cancellationToken);
    public static bool IsDBNull(this DbDataReader reader, string name)
        => reader.IsDBNull(reader.GetOrdinal(name));
    public static bool GetBoolean(this DbDataReader reader, string name)
        => reader.GetBoolean(reader.GetOrdinal(name));
    public static byte GetByte(this DbDataReader reader, string name)
        => reader.GetByte(reader.GetOrdinal(name));
    public static long GetBytes(this DbDataReader reader, string name, long dataOffset, byte[] buffer, int bufferOffset, int length)
        => reader.GetBytes(reader.GetOrdinal(name), dataOffset, buffer, bufferOffset, length);
    public static char GetChar(this DbDataReader reader, string name)
        => reader.GetChar(reader.GetOrdinal(name));
    public static long GetChars(this DbDataReader reader, string name, long dataOffset, char[] buffer, int bufferOffset, int length)
        => reader.GetChars(reader.GetOrdinal(name), dataOffset, buffer, bufferOffset, length);
    public static DbDataReader GetData(this DbDataReader reader, string name)
        => reader.GetData(reader.GetOrdinal(name));
    public static string GetDataTypeName(this DbDataReader reader, string name)
        => reader.GetDataTypeName(reader.GetOrdinal(name));
    public static DateTime GetDateTime(this DbDataReader reader, string name)
        => reader.GetDateTime(reader.GetOrdinal(name));
    public static decimal GetDecimal(this DbDataReader reader, string name)
        => reader.GetDecimal(reader.GetOrdinal(name));
    public static double GetDouble(this DbDataReader reader, string name)
        => reader.GetDouble(reader.GetOrdinal(name));
    public static Type GetFieldType(this DbDataReader reader, string name)
        => reader.GetFieldType(reader.GetOrdinal(name));
    public static T GetFieldValue<T>(this DbDataReader reader, string name)
        => reader.GetFieldValue<T>(reader.GetOrdinal(name));
    public static float GetFloat(this DbDataReader reader, string name)
        => reader.GetFloat(reader.GetOrdinal(name));
    public static Guid GetGuid(this DbDataReader reader, string name)
        => reader.GetGuid(reader.GetOrdinal(name));
    public static short GetInt16(this DbDataReader reader, string name)
        => reader.GetInt16(reader.GetOrdinal(name));
    public static int GetInt32(this DbDataReader reader, string name)
        => reader.GetInt32(reader.GetOrdinal(name));
    public static long GetInt64(this DbDataReader reader, string name)
        => reader.GetInt64(reader.GetOrdinal(name));
    public static Stream GetStream(this DbDataReader reader, string name)
        => reader.GetStream(reader.GetOrdinal(name));
    public static string GetString(this DbDataReader reader, string name)
        => reader.GetString(reader.GetOrdinal(name));
    public static TextReader GetTextReader(this DbDataReader reader, string name)
        => reader.GetTextReader(reader.GetOrdinal(name));
}
@roji

This comment has been minimized.

Copy link
Member Author

@roji roji commented Aug 4, 2018

It could definitely be extensions, but is there a good reason to have something as an extension when it could be built-in? It's a bit like how the async methods always have their non-virtual overload without a cancellation token, which delegates to the virtual overload that does...

@benaadams

This comment has been minimized.

Copy link
Collaborator

@benaadams benaadams commented Aug 4, 2018

is there a good reason to have something as an extension when it could be built-in?

The only get compiled on use (rather than whole class for instance level) and they can be added downlevel via nuget; whereas changing the base needs a rev to the runtime.

If they are only using the public method and non-virtual, is there any reason to be an instance method?

@roji

This comment has been minimized.

Copy link
Member Author

@roji roji commented Aug 4, 2018

is there a good reason to have something as an extension when it could be built-in?

The only get compiled on use (rather than whole class for instance level)

Are you talking about JIT compilation? Doesn't that only happen on first use of the method?

and they can be added downlevel via nuget; whereas changing the base needs a rev to the runtime.

That's true, but which nuget would you put them in? Would you expect users to depend on a nuget just to get this kind of utility? The last could possibly be mitigated by having some sort of general "BCL extensions package"... I also don't particularly mind for this to only make it into the next .NET Core (and .NET Standard).

Also, it's a bit concerning to see core/fundamental APIs migrate out of the BCL and into extension nugets just because they can be implemented as extensions... It hurts discoverability (doesn't show up in IntelliSense unless you've referenced the nuget), the extensions don't show up on the DbDataReader docs (I think), etc.

If they are only using the public method and non-virtual, is there any reason to be an instance method?

Nothing beyond general object-oriented design principles... From the user's point of view both GetInt32(int) and GetInt32(string) are equivalent/similar, so it seems like they should be defined in the same place.

I guess it's a very general design discussion... It feels like I've seen many utility overloads in the BCL as instance methods that could be extension methods (non-virtual, using public APIs), the async overload without cancellation token is just one example. We also have cases of extension methods in external nugets, but that seems to be a way to add necessary APIs without revving the runtime version - but these proposed APIs aren't exactly necessary/urgent.

@benaadams

This comment has been minimized.

Copy link
Collaborator

@benaadams benaadams commented Aug 4, 2018

is there a good reason to have something as an extension when it could be built-in?

The only get compiled on use (rather than whole class for instance level)

Are you talking about JIT compilation? Doesn't that only happen on first use of the method?

I think its more of a problem for generic classes; and may depend it if its being distrubuted as part of the runtime (AoT/Crossgen/Ready-To-Run)? Not sure of the specifics here /cc @jkotas

That's true, but which nuget would you put them in?

System.Data.Extensions

It hurts discoverability

Span is probably a good example as most of its extra methods are in Extension Methods both listed in docs on the Span page and come up as methods in IntelliSense as they are in the same namespace.

The extensions can be inboxed for a runtime/standard to be discoverable by default (without reference); and available as a Nuget to immediately be available on every runtime for maximum reach (netcore, netfx, Mono, Unity, iOS, Android, etc).

Also, it's a bit concerning to see core/fundamental APIs migrate out of the BCL and into extension nugets just because they can be implemented as extensions...

but these proposed APIs aren't exactly necessary/urgent.

Are they core/fundamental or are they just convenience wrappers over the current public api?

If they were virtual; then they'd definitely need to be on the type.

If they are non-virtual and just calling public methods; other than a tooling issue (discovery), what's the advantage of forcing every runtime to copy paste them and forcing users to wait for that runtime to rev to a version that includes the apis? (e.g. .NET Core 3.0; some unknown future version of Mono, some unknown future version of .NET Standard) when it could be a nuget targeting current NET Standard and available everywhere on the first release?

I've seen many utility overloads in the BCL as instance methods that could be extension methods (non-virtual, using public APIs), the async overload without cancellation token is just one example.

If they are added at the same time; is there an advantage to creating an extension class with single methods? (Maybe so you know its just a call through, rather than its own implementation).

Whereas this is an additional 24 methods to a preexisting class, all public api wrappers, so has more bulk to the extension class?

Not sure if there is specific guidance on non-virtual base class instance vs extension somewhere? /cc @terrajobst

@jkotas

This comment has been minimized.

Copy link
Member

@jkotas jkotas commented Aug 4, 2018

I think its more of a problem for generic classes; and may depend it if its being distrubuted as part of the runtime (AoT/Crossgen/Ready-To-Run)? Not sure of the specifics here /cc @jkotas

Right, this is a problem of the frequently used generic types like Dictionary. I do not think this concern applies here. I think these can be instance methods just fine.

@roji

This comment has been minimized.

Copy link
Member Author

@roji roji commented Aug 4, 2018

That's true, but which nuget would you put them in?

System.Data.Extensions

Right, so we're talking about a new nuget here... Do we really want to basically start having a nuget extensions package for each BCL namespace? More nugets for people to discover and reference, more potential for the dependency hell inherent in additional dependencies...

Span is probably a good example as most of its extra methods are in Extension Methods both listed in docs on the Span page and come up as methods in IntelliSense as they are in the same namespace.
The extensions can be inboxed for a runtime/standard to be discoverable by default (without reference); and available as a Nuget to immediately be available on every runtime for maximum reach (netcore, netfx, Mono, Unity, iOS, Android, etc).

That's indeed a good example. But with Span an important goal was to make the new methods available backwards, for all existing platforms, and that can obviously be done only via extensions. Here I don't think we have this goal at all - it's a convenience wrapper that can be added going forward.

but these proposed APIs aren't exactly necessary/urgent.

Are they core/fundamental or are they just convenience wrappers over the current public api?

I don't think that convenience wrappers can't be core/fundamental. The point is that the class in question, DbDataReader, is definitely core/fundamental, and getting a column value by name seems to be completely basic functionality that shouldn't involve another nuget. The case of the async cancellation token overloads comes to mind again (I know I'm repeating myself...).

I've seen many utility overloads in the BCL as instance methods that could be extension methods (non-virtual, using public APIs), the async overload without cancellation token is just one example.

If they are added at the same time; is there an advantage to creating an extension class with single methods? (Maybe so you know its just a call through, rather than its own implementation).

I don't see the need for users to know whether that something is just a call through or not... From the users' perspective that seems like an unimportant implementation detail.

To summarize, it seems that perf isn't an argument in either direction. Adding this as extension methods via an external nuget is problematic in that there's an extra dependency and in that discoverability is hurt, whereas adding this as instance methods only makes it available going forward (and not to older versions).

Seems like it would indeed be good to have some input from someone like @terrajobst...

@jkotas

This comment has been minimized.

Copy link
Member

@jkotas jkotas commented Aug 4, 2018

Do we really want to basically start having a nuget extensions package for each BCL namespace?

We have been on this plan (adding NuGets with extensions) for a while in the past. It did not work well. We are not doing that anymore.

Adding this as instance methods only makes it available going forward

Correct. We are fine with this constrain.

@benaadams

This comment has been minimized.

Copy link
Collaborator

@benaadams benaadams commented Aug 4, 2018

Instance methods it is then :)

@roji do you want to specify the exact api shape (new methods) in the summary (first item)?

@roji

This comment has been minimized.

Copy link
Member Author

@roji roji commented Aug 4, 2018

Sure, you basically did all the work for me :)

@roji

This comment has been minimized.

Copy link
Member Author

@roji roji commented Aug 4, 2018

@jkotas out of curiosity, what were the specific issues with the extension NuGet approach that made you drop it? Just interested if there's an additional con I haven't thought of.

@jkotas

This comment has been minimized.

Copy link
Member

@jkotas jkotas commented Aug 4, 2018

We typically needed to fold the addition back into the platform, e.g. because of it is too valuable API to not have by default; or to optimize the implementation in a way that is not possible to when it is an extension. The end result was a lot of small .dlls (you cannot get rid of the extra NuGet/assembly even once it is folded into the platform), and version mixes that were impossible to reason about during roll-forward.

@karelz karelz added this to the Future milestone Aug 15, 2018
@TheBlueSky

This comment has been minimized.

Copy link
Contributor

@TheBlueSky TheBlueSky commented Oct 1, 2018

@jkotas & @karelz, I'm curious, why this one is added to Future milestone? This is an easy implementation that does not break people and adds value. There is no project I recently worked on without adding my own Get*(string column) extensions :)

I can implement this if you're interested.

@jkotas

This comment has been minimized.

Copy link
Member

@jkotas jkotas commented Oct 1, 2018

https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md describes the process to add new APIs.

This one is on System.Data area owners to move this forward.

cc @divega @ajcvickers @keeratsingh @AfsanehR @David-Engel

@TheBlueSky

This comment has been minimized.

Copy link
Contributor

@TheBlueSky TheBlueSky commented Oct 1, 2018

OK, thanks. I'll wait for a comment from the owners then :)

@ajcvickers

This comment has been minimized.

Copy link
Member

@ajcvickers ajcvickers commented Oct 1, 2018

This looks like a good addition to me. I don't see any reason we wouldn't take a well-written PR for it. /cc @roji @divega

@jkotas

This comment has been minimized.

Copy link
Member

@jkotas jkotas commented Oct 1, 2018

@ajcvickers If you like the proposal, you should mark it as api-ready-for-review and get it approved first (see https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md).

@roji

This comment has been minimized.

Copy link
Member Author

@roji roji commented Oct 1, 2018

I was intending to submit a PR on this in a few weeks, but if someone else has time that's great.

@karelz

This comment has been minimized.

Copy link
Member

@karelz karelz commented Oct 1, 2018

@ajcvickers @David-Engel @keeratsingh @AfsanehR can you please agree on the API shape from System.Data ownership point of view and if you do, mark it as api-ready-for-review as suggested above?
We want to avoid surprise API changes after community invested time into PR submission. Thanks!

@ajcvickers

This comment has been minimized.

Copy link
Member

@ajcvickers ajcvickers commented Oct 1, 2018

@karelz I would like to hear back from @divega since while I own from an engineering side, he is the PM owner for .NET data.

@David-Engel

This comment has been minimized.

Copy link

@David-Engel David-Engel commented Oct 12, 2018

I'll defer API suggestions in System.Data to @divega also. We service the code, but he should review API surface area suggestions.

@divega

This comment has been minimized.

Copy link

@divega divega commented Oct 12, 2018

@roji @ajcvickers @David-Engel, Sorry I didn't respond before. I am a bit surprised I didn't see all the mentions 😄

I looked at the signatures, they seem correct, and I am confident that if there is anything missing or incorrect @roji will find it while his coding it. So @roji sure, you can send a PR 😄

@divega

This comment has been minimized.

Copy link

@divega divega commented Oct 12, 2018

BTW, I think this is a small improvement, but also low hanging fruit. I only have a couple of comments I would like to share on the proposal, and if anyone has feedback on these, I am interested in learning what you think:

  • Even after this improvemnt, this type still feels old odd. If I am writing an application, I am personally not going to want to use this API type directly. And if I am writing and O/RM, I would probably avoid the proposed string methods in most cases for performance (I'd rather build a materializer using GetOrdinal and then avoid repeating the lookup for each field in each row). I wonder what kind of data reader abstraction we could design if we started again in 2019, given that we have now figured out generics, IEnumerable, Nullable<T>, streaming with reusable ValueTask<T> (similar to the IAsyncEnumerable<T> proposal), and even the TryGet pattern?

  • It also feels odd that we are extending this API over the ordinal vs. field name pivot, when we have recently avoided doing it in other possible dimensions. For example, if you want to get a field value asynchronously, your only option is to use the generic GetFieldValueAsync. We added IsDBNullAsync because it is necessary (you have to call it defensively before GetFieldValueAsync, yet another usability issue in the API), but we didn't add GetBooleanAsync, or GetByteAsync, we haven't updated the equivalent APIs in DbDataRecord for several years, etc. Though I totally get that needing to get a field value asynchronously is less common than doing it synchronously (you should only ever do it if you are reading with SequentialAccess), it is weird that everything is so inconsistent.

@karelz

This comment has been minimized.

Copy link
Member

@karelz karelz commented Oct 12, 2018

@divega if you are OK with the API, please get formal API approval (mark it as api-ready-for-review & join us in Tue meeting - cc Immo)
Once approved, it will be good time to put up PR (just to avoid wasting @roji's time in case the API approval asks for adjustments ...)

@benaadams

This comment has been minimized.

Copy link
Collaborator

@benaadams benaadams commented Oct 13, 2018

If I am writing an application, I am personally not going to want to use this API directly.

For single row data this is a bit of clunky usage pattern

int field0 = reader.GetInt32(reader.GetOrdinal("field0"));
// or
int field1Index = reader.GetOrdinal("field1");
int field1 = reader.GetOrdinal(field1Index);

vs the proposed

int field0 = reader.GetInt32("field0");

The current named column option as it goes via boxing gives unhelpful errors when casting:

long field0 = (long)reader["field0"];

InvalidCastException: Specified cast is not valid. (as field is int and can't be cast boxed in object to long)

I wonder what kind of data reader abstraction we could design if ...

Interesting 😄

We added IsDBNullAsync because it is necessary (you have to call it defensively before GetFieldValueAsync

Would using nullables with the generic be valid for this? (Although not helpful for string necessarily)

int field0 = await reader.GetFieldValueAsync<int?>(0);
@Wraith2

This comment has been minimized.

Copy link
Collaborator

@Wraith2 Wraith2 commented Mar 5, 2019

Sure, sounds like a good place to put them.

@roji

This comment has been minimized.

Copy link
Member Author

@roji roji commented Mar 11, 2019

@TheBlueSky assuming you're still interested, can you please submit a PR removing the new string overloads out of DbDataReader and reintroducing them as extension methods?

@TheBlueSky

This comment has been minimized.

Copy link
Contributor

@TheBlueSky TheBlueSky commented Mar 11, 2019

@roji, yeah, I'll do that.

For the record, I don't see the benefit of having them as extensions if they are going to be baked in and not distributed as a NuGet package. I always thought of extension methods as a way to provide additional functionality to a class where there is a limitation or to provide functionalities that are specific to the extender. In this case, it's just to prevent breaking a hypothetical 3rd party library.

I hope this case won't set a precedent for how future Core FX APIs are decided.

@divega

This comment has been minimized.

Copy link

@divega divega commented Mar 11, 2019

@TheBlueSky if you read above in the thread, there are possible advantages of extension methods. But the real reason we switch to extension methods is compatibility with reflection code that assumes there is a single instance method with each name.

@roji

This comment has been minimized.

Copy link
Member Author

@roji roji commented Mar 15, 2019

@TheBlueSky we got approval for changing this as described in the proposal - will you be able to submit a PR for that early next week? This includes both the switch to extension methods and the additional async methods.

This really needs to happen soon as it's breaking various users, if you're too busy please let me know and I'll take care of it.

Thanks!

@TheBlueSky

This comment has been minimized.

Copy link
Contributor

@TheBlueSky TheBlueSky commented Mar 15, 2019

@roji, will do that.

TheBlueSky added a commit to TheBlueSky/corefx that referenced this issue Mar 18, 2019
Also, add `Task<T> GetFieldValueAsync<T>` and `Task<bool> IsDBNullAsync` overloads.

Addresses dotnet#31595
@TheBlueSky

This comment has been minimized.

Copy link
Contributor

@TheBlueSky TheBlueSky commented Mar 18, 2019

@roji, I created a PR; however, although builds and tests pass, some checks fail at "Send to Helix" step. Could you have a look.

@Grauenwolf

This comment has been minimized.

Copy link

@Grauenwolf Grauenwolf commented Mar 19, 2019

TheBlueSky added a commit to TheBlueSky/corefx that referenced this issue Mar 19, 2019
Also, add `GetFieldValueAsync<T>()` and `IsDBNullAsync()` string overloads.

Addresses dotnet#31595
@roji

This comment has been minimized.

Copy link
Member Author

@roji roji commented Mar 19, 2019

@Grauenwolf I assume you're reacting to #31595 (comment)? If so yeah, I mixed up two discussions and my comment there wasn't relevant (I've already added strike-through).

TheBlueSky added a commit to TheBlueSky/corefx that referenced this issue Mar 19, 2019
Also, add `GetFieldValueAsync<T>()` and `IsDBNullAsync()` string overloads.

Addresses dotnet#31595
TheBlueSky added a commit to TheBlueSky/corefx that referenced this issue Mar 23, 2019
Also, add `GetFieldValueAsync<T>()` and `IsDBNullAsync()` string overloads.

Addresses dotnet#31595
TheBlueSky added a commit to TheBlueSky/corefx that referenced this issue Mar 27, 2019
Also, add `GetFieldValueAsync<T>()` and `IsDBNullAsync()` string overloads.

Addresses dotnet#31595
TheBlueSky added a commit to TheBlueSky/corefx that referenced this issue Mar 27, 2019
Also, add `GetFieldValueAsync<T>()` and `IsDBNullAsync()` string overloads.

Addresses dotnet#31595
roji added a commit that referenced this issue Mar 29, 2019
Also, add `GetFieldValueAsync<T>()` and `IsDBNullAsync()` string overloads.

Addresses #31595
@divega

This comment has been minimized.

Copy link

@divega divega commented Apr 2, 2019

@roji I think this issue was addressed. Can we close it now?

@roji

This comment has been minimized.

Copy link
Member Author

@roji roji commented Apr 2, 2019

Yes, finally... :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.