-
Notifications
You must be signed in to change notification settings - Fork 862
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
'DynamoDbContext.LoadAsync' does not respect 'DynamoDBOperationConfig.IndexName' #1748
Comments
Hi @julealgon, Good afternoon. Please refer to the page .NET: Object Persistence Model -> Supported Data Types for the list of supported data types. Looks like As a workaround, you may use using Amazon.DynamoDBv2;
using Amazon.DynamoDBv2.DataModel;
using Amazon.DynamoDBv2.DocumentModel;
using System;
namespace DynamoDbSecondaryIndexTest1748
{
class Program
{
static void Main(string[] args)
{
AmazonDynamoDBClient amazonDynamoDBClient = new AmazonDynamoDBClient();
DynamoDBContext dynamoDBContext = new DynamoDBContext(amazonDynamoDBClient);
var loadResult = dynamoDBContext.LoadAsync<Something>("75f05566-2209-4f3d-ba5a-cfd9da5eab62", new DynamoDBOperationConfig { IndexName = "Name-index" }).GetAwaiter().GetResult();
Console.WriteLine("Loaded Result: " + loadResult);
}
}
[DynamoDBTable("TestTable")]
internal sealed class Something
{
[DynamoDBHashKey]
[DynamoDBProperty(typeof(GuidTypeConverter))]
public Guid Id { get; set; }
[DynamoDBGlobalSecondaryIndexHashKey]
public string Name { get; set; }
public override string ToString()
{
return $"[Id: {Id}, Name: {Name}]";
}
}
public class GuidTypeConverter : IPropertyConverter
{
public DynamoDBEntry ToEntry(object value)
{
if(value == null || !(value is Guid)) throw new ArgumentOutOfRangeException();
DynamoDBEntry entry = new Primitive
{
Value = ((Guid)value).ToString()
};
return entry;
}
public object FromEntry(DynamoDBEntry entry)
{
Primitive primitive = entry as Primitive;
Guid guidValue;
if (primitive == null || !(primitive.Value is String) || string.IsNullOrEmpty((string)primitive.Value) || !Guid.TryParse((string)primitive.Value, out guidValue))
throw new ArgumentOutOfRangeException();
return guidValue;
}
}
} NOTE: The above code should be used only for reference. Kindly validate and test the Please let me know if this helps and if this issue could be closed. Thanks, |
@ashishdhingra I believe you might have misunderstood my request here. Please note that The problem here happens with the global secondary index, which has a I kindly request that you take another look at this. Even if the page you provided doesn't mention |
Also @ashishdhingra , I'd suggest to avoid providing example source code where you block on async code, as that is a bad practice that can lead to hard to debug problems in .Net. In your sample People not aware of the issues might end up naively copying those code sections into their code. |
Hi @julealgon, Good morning. Thanks for your inputs. I used your code exactly where it tries to use the secondary index, to reproduce the issue. When not using the type converter, the In the mean while, in order to get proper understanding of the issue, please share the following:
dynamoDBContext.LoadAsync<Something>("75f05566-2209-4f3d-ba5a-cfd9da5eab62", new DynamoDBOperationConfig { IndexName = "Name-index" }) Regarding the example code where I'm blocking Thanks, |
@ashishdhingra , can you clarify on this?
Do you mean you can use
Primary index:
Global Secondary index:
This configuration allows me to:
I cannot currently use
Affirmative. That call produces ...
[DynamoDBHashKey]
public Guid Id { get; set; }
[DynamoDBGlobalSecondaryIndexHashKey]
public string? Name { get; set; }
... nor should it require the creation of a converter from |
I performed another test on my end which I figured could potentially fix the issue. I realized that the [DynamoDBHashKey]
public Guid Id { get; set; }
[DynamoDBGlobalSecondaryIndexHashKey("Name-index")]
public string? Name { get; set; } However, I still get the same error when attempting to use |
Hi @julealgon, Upon further investigating and using the code below (NOTE: To keep things simple, I used using Amazon.DynamoDBv2;
using Amazon.DynamoDBv2.DataModel;
using Amazon.DynamoDBv2.DocumentModel;
using System;
namespace DynamoDbSecondaryIndexTest1748
{
class Program
{
static void Main(string[] args)
{
AmazonDynamoDBClient amazonDynamoDBClient = new AmazonDynamoDBClient();
DynamoDBContext dynamoDBContext = new DynamoDBContext(amazonDynamoDBClient);
var loadResultSecondary = dynamoDBContext.LoadAsync<Something>("87a8f81a-9fe9-48cb-a7c0-a7a6788d91d3", new DynamoDBOperationConfig { IndexName = "Name-index" }).GetAwaiter().GetResult();
Console.WriteLine("Loaded Result: " + loadResultSecondary);
}
}
[DynamoDBTable("TestTable")]
internal sealed class Something
{
[DynamoDBHashKey]
public Guid? Id { get; set; }
[DynamoDBGlobalSecondaryIndexHashKey]
public string Name { get; set; }
public override string ToString()
{
return $"[Id: {Id}, Name: {Name}]";
}
}
} it gave the mentioned error, whether I used
I was able to trace the call to TryTo. Basically when you pass string value, it tries to use The workaround here is to execute the call Please let me know if this works for you and let me know if this issue could be closed. Thanks, |
Why though? It shouldn't be trying to do that, since the index I'm querying does not have a
This is not a workaround, this is a different query altogether. Please note that the I hope that clarifies the issue. Let me know if you need additional info. |
Hi @julealgon, The For the 2nd question, DynamoDB SDK's I'm not sure if for your use case (per your 2nd question), you intend to use Hope this helps. Thanks, |
Incorrect. The problem I'm having is not with the primary key (which is mapped as
I understand that, however the
I understand the error, but I don't understand why it is attempting to perform that conversion in the first place. The type it should be attempting to cast to for the secondary index is
Now this is where it gets incredibly misleading then. The
If it truly is referring to the primary hashkey, then the API is incredibly misleading an needs work, in my opinion. Especially when directly compared to "sibling" APIs like
From the API surface, it would appear that |
Hi @julealgon, With all above inputs I will work with developer to have a look at it, even though it might be a .NET API documentation update. Kindly notice that the GetItem service API call returns single item, as opposed to Query service API call which returns multiple items. Hence GetItem should really work on primary key which is unique for all items. Please validate if this explains the behaviour of LoadAsync. NOTE:
I will add Thanks, |
Again, while changing the documentation to make this behavior explicit is a valid "fix", I personally think it is a bad option in this case as the APIs will "appear" to behave the same but work differently at the end of the day, and this is bad for consumers. Obviously, this is up to you, just wanted to make my position clear here.
Perhaps we are confusing "primary/secondary" with "hash/sort" keys here? My understanding was that secondary indexes also had unique items under their respective hash keys, so the approach shouldn't change regardless if we are talking about single-result Please do correct me if I'm wrong here as I just started interfacing with Dynamo.
I never noticed the XML docs for the non-async call as I was always using the async version by default. This documentation does seem to suggest it only works on the primary index (having the If the documentation is really correct here and this is not a bug, then I would strongly recommend removing the capability of passing the index name in the
I appreciate your support on this Ashish. |
@julealgon Adding more context here. As @ashishdhingra mentioned I agree the parameter name on the As for the idea of making At this point the best we can do is improve the documentation. Anything else causes serious breaking changes. |
Thank you @normj for the information.
If you are not willing to change the behavior of the API, perhaps you could introduce new overloads, with a tighter config model (containing only the settings that make sense for any given operation) while retaining the old overload as-is, but marking it as Another potential option would be to actually throw an exception at runtime when an invalid setting is passed into the call. I'd rather get an exception that tells me why that setting cannot be used, than get "wrong results" as is happening right now. Anyways, very unfortunate to have such a misleading API stuck in place. I'll probably need a significant comment block in my code to explain this anomaly and justify using the |
Just spent a day banging my head into the wall trying to |
We have noticed this issue has not received attention in 1 year. We will close this issue for now. If you think this is in error, please feel free to comment and reopen the issue. |
From my perspective, this is still valid, even if its a documentation-only change. |
Needs review on how we could implement this documentation change. |
Wow, I just spent hours trying to work out what I was doing wrong! This really needs to be included in the documentation if it's not going to be fixed. It would've saved me so much headache! |
For the short term, we've clarified the Longer term, we would like to make the change @julealgon suggested above - move away from the "shared" |
We've merged #3421 into the We've marked the methods that take We're still working on getting preview packages published, you can track overall V4 progress in #3362. We'll leave this issue open until at least V4 hits GA. |
The V4 preview is out, please let us know if you find any issues with the new operation-specific config objects. |
Comments on closed issues are hard for our team to see. |
Description
When using the
LoadAsync
method onIDynamoDbContext
to load an entity using a secondary global index, I'm getting unexpected behavior: anInvalidCastException
is being thrown.Switching the exact same call to a
QueryAsync
fixes the problem.I believe
LoadAsync
is not respecting theIndexName
property on the configuration object and is using the primary index for all searches. Either the API is incredibly misleading (as it allows you to pass this unused value) or there is a bug there.Reproduction Steps
Guid
propertystring
propertyLoadAsync
with astring
key and passnew DynamoDBOperationConfig { IndexName = <Name_Of_Secondary_Index> }
as optionsInvalidCastException
is thrownLoadAsync
call withQueryAsync
, and it works as expected:Logs
Environment
AWSSDK.Core
Version="3.5.1.39"AWSSDK.DynamoDBv2
Version="3.5.3.3"Resolution
I belive
LoadAsync
should be changed to respect the index being passed as a parameter and use theobject hashKey
against that.This is a 🐛 bug-report
The text was updated successfully, but these errors were encountered: