Add string overloads to DbDataReader.Get*()

[roji]

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 #25297). 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 dotnet/corefx#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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

Instance methods it is then :)

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

[roji]

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

[roji]

@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]

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.

[TheBlueSky]

@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]

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]

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

[ajcvickers]

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]

@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]

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

[karelz]

@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]

@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]

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

[divega]

@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]

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]

@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]

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);

[divega]

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

Yes, I am not disagreeing that it is an improvement for some scenario. I meant I would not want to use DbDataReader without an O/RM or micro-O/RM in an application at all 😄

Would using nullables with the generic be valid for this?

Maybe. I am honestly not sure what new trade offs we would make. Perhaps some things would remain the same. Are non-generic methods for common (even if not universally supported) types easier to use anyway? Is the distinction between null and DBNull a necessary evil?

[divega]

@karelz done, thanks!

[TheBlueSky]

@divega and @roji, can I give this one a shot?

[TheBlueSky]

@roji, I am interested in making this change.

I have a question related to EF Core issue. What makes this "special"? Is it because we got to know about it, or is there a rule on when and where we can add new APIs? I mean any additional API in any random Core FX class can break someone's library.

[roji]

@TheBlueSky please hold off for a day or two before actually implementing anything, we're still discussing the level of backwards compatibility we want to provide.

I have a question related to EF Core issue. What makes this "special"? Is it because we got to know about it, or is there a rule on when and where we can add new APIs? I mean any additional API in any random Core FX class can break someone's library

You're right, and I think there aren't any completely clear-cut rules about this. It's more of a pragmatic choice trying to consider the risk of breakage - and in this case we do have the indicator that a major O/RM was broken by it. It's definitely not an easy decision.

[roji]

Discussed again with @divega and we feel that this proposal should indeed include the string overloads for GetFieldValueAsync() and IsDBNullAsync(), returning Task. The idea here would be to maintain API consistency - if all other column getters have these overloads, we don't see why this one shouldn't. We think that other overloads returning ValueTask (and possibly behaving better with regards to null) can and should be introduced, but see this as a separate question - those overloads would be introduced with new names in any case.

Otherwise there's still the question of implementing via methods on DbDataReader or extensions.

@divega can you please mark ready-for-review?

[divega]

@roji I have added the label before I forget, but could you please make sure before Tuesday we have the right set of APIs for review on the first comment? E.g. the extension methods, including the two existing async methods? I can close the issue I created for the async ones afterwards.

I was also re-reading the initial comments from @benaadams and now that we have to do extension methods anyway, I realize the benefits he mentioned will be cool 😄

[divega]

@roji let me know when we can review and I will ping @terrajobst to get a slot to re-review this one.

[roji]

@divega sorry this took so long. I have updated the proposal above (both for extensions and for adding back the two async overloads), this should be ready for review again.

FWIW I'm still not entirely convinced about the extensions, but it's not very important :) When the option of extensions was originally discussed, I think one of the main points was bringing this functionality to older TFMs as well by putting the extensions in an external nuget. We're not going to do that, so I'm not sure there's any advantage to extensions beyond preventing breakage. But I admit there's no big downside either :)

[Wraith2]

The extensions could always be put into a source nuget for older TFMs if needed. I know i've written those extensions several times so people may not need them in older projects but if we prevent them having to find and pull in those snippets of code in new projects i think it's a success.

[karelz]

@divega removing api-approved as you apparently need another round of API review ...

[divega]

@roji @Wraith2 I believe since DbDataReaderExtensions already exists and ships in the System.Data.Common package as a real implementation (as opposed to type-forwarding), these extension methods will probably be available everywhere.

Anyway, we can go over the details in the API review.

[Wraith2]

Sure, sounds like a good place to put them.

[roji]

@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]

@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]

@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]

@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]

@roji, will do that.

[TheBlueSky]

@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]

the returned Task can already be cached and does not involve any perf hit.

Are you sure about that? Wouldn't you have to cache a separate Task object for every possible value?

…

On Sat, Jan 12, 2019 at 9:52 AM Shay Rojansky ***@***.***> wrote: @Grauenwolf <https://github.com/Grauenwolf> you're right that SequentialAccess is important (for ORMs and otherwise) - the above comment was only to hypothesize on why no type-specific getters (e.g. GetBoolean()) exist in the current API. I don't think it's very important - IMHO since the generic GetFieldValue<T>() and GetFieldValueAsync<T>() already exist, I don't see any reason to introduce any new type-specific getters. Regarding Task vs. ValueTask, it's true that a method similar to GetFieldValueAsync<T>() returning a VauleTask (as opposed to Task) could be useful. It's worth noting that this would provide a perf improvement only when actual I/O needs to be done to read the column - if it's already in memory (as it many times is), the returned Task can already be cached and does not involve any perf hit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/dotnet/corefx/issues/31595#issuecomment-453767528>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKIqTcpuKnC_KKPl6mgRtwa7rw3OYKqBks5vCiDhgaJpZM4Vu275> .

[roji]

@Grauenwolf I assume you're reacting to https://github.com/dotnet/corefx/issues/31595#issuecomment-453767528? If so yeah, I mixed up two discussions and my comment there wasn't relevant (I've already added strike-through).

[divega]

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

[roji]

Yes, finally... :)