feat: data error response update #176
Conversation
| @@ -1,8 +1,27 @@ | |||
| namespace Momento.Sdk.Responses; | |||
|
|
|||
| using Momento.Sdk.Exceptions; | |||
|
|
|||
| public class CacheDeleteResponse | |||
There was a problem hiding this comment.
can't remember if we decided we were going to make these abstract, nbd though
There was a problem hiding this comment.
We did, and I did for the control responses but not the data ones. Fixed in efab849.
| public IEnumerable<string?> Strings() | ||
| { | ||
| return Responses.Select(response => response.String()); | ||
| public IEnumerable<CacheGetStatus> Status |
There was a problem hiding this comment.
@malandis you should look at this part. i'm not super familiar with this part of the API but some random thoughts I have are:
- We have been getting rid of the enums in other places, and I had assumed that would happen for CacheGet too, but it seems like it's not happening here.
- do we really need 3 separate methods for this? would it not be okay to just have a single enumerable that contains the polymorphic parent type?
- in the loops below it looks like we are assuming that all of the responses are
Success, but I don't think there is any guarantee that that is actually true?
There was a problem hiding this comment.
- You could. I included methods to parallel the unary
Get: get the status, get the payload. Since this is a batch operation I also included to get the individual items - That's correct for the way this is implemented now. If this were a single RPC then there would only be on
Success
| public class Success : CacheGetResponse | ||
| { | ||
| public CacheGetStatus Status { get; } | ||
| protected readonly ByteString? value; |
There was a problem hiding this comment.
I would have expected us to have sub classes for Hit and Miss here, and thus avoid the nullable stuff like we were doing elsewhere.
There was a problem hiding this comment.
You're totally right . . . I was just in a rhythm :-) Fixed in 0aa3060.
There was a problem hiding this comment.
That commit looks a great improvement but the field is still nullable here... it shouldn't need to be anymore, should it?
| public IEnumerable<string?> Strings() | ||
| { | ||
| return Responses.Select(response => response.String()); | ||
| public IEnumerable<CacheGetStatus> Status |
There was a problem hiding this comment.
- You could. I included methods to parallel the unary
Get: get the status, get the payload. Since this is a batch operation I also included to get the individual items - That's correct for the way this is implemented now. If this were a single RPC then there would only be on
Success
| byte[]? setValue = (await client.GetAsync(cacheName, key)).ByteArray; | ||
| Assert.Equal(value, setValue); | ||
| CacheGetResponse response = await client.GetAsync(cacheName, key); | ||
| if (response is CacheGetResponse.Success goodResponse) { | ||
| byte[]? setValue = goodResponse.ByteArray; | ||
| Assert.Equal(value, setValue); | ||
| } |
There was a problem hiding this comment.
The previous tests assumed a Success. Otherwise they would throw an exception and the test would fail. In this version if the response is not a success, the test still passes. So we need an explicit check to assert Success
There was a problem hiding this comment.
I noticed that I was potentially causing false positives in those spots too, thanks. I've updated these to use a direct cast outside of an if, so the tests will fail if the subtype we're expecting isn't the one we get. This should address the remainder of your comments as well.
| setValue = (await client.GetAsync(cacheName, key)).ByteArray; | ||
| Assert.Equal(value, setValue); | ||
| response = await client.GetAsync(cacheName, key); | ||
| if (response is CacheGetResponse.Success anotherGoodResponse) { | ||
| byte[]? setValue = anotherGoodResponse.ByteArray; | ||
| Assert.Equal(value, setValue); | ||
| } |
| string? setValue = (await client.GetAsync(cacheName, key)).String(); | ||
| Assert.Equal(value, setValue); | ||
| CacheGetResponse response = await client.GetAsync(cacheName, key); | ||
| if (response is CacheGetResponse.Success goodResponse) { | ||
| string? setValue = goodResponse.String(); | ||
| Assert.Equal(value, setValue); | ||
| } |
| setValue = (await client.GetAsync(cacheName, key)).String(); | ||
| Assert.Equal(value, setValue); | ||
| response = await client.GetAsync(cacheName, key); | ||
| if (response is CacheGetResponse.Success anotherGoodResponse) { | ||
| string? setValue = anotherGoodResponse.String(); | ||
| Assert.Equal(value, setValue); | ||
| } |
| byte[]? setValue = (await client.GetAsync(cacheName, key)).ByteArray; | ||
| Assert.Equal(value, setValue); | ||
| CacheGetResponse response = await client.GetAsync(cacheName, key); | ||
| if (response is CacheGetResponse.Success goodResponse) { | ||
| byte[]? setValue = goodResponse.ByteArray; | ||
| Assert.Equal(value, setValue); | ||
| } |
| setValue = (await client.GetAsync(cacheName, key)).ByteArray; | ||
| Assert.Equal(value, setValue); | ||
| response = await client.GetAsync(cacheName, key); | ||
| var anotherGoodResponse = (CacheGetResponse.Success)response; | ||
| byte[]? anotherSetValue = anotherGoodResponse.ByteArray; | ||
| Assert.Equal(value, anotherSetValue); |
| await Assert.ThrowsAsync<Momento.Sdk.Exceptions.TimeoutException>(async () => await simpleCacheClient.GetBatchAsync(cacheName, keys)); | ||
| CacheGetBatchResponse response = await simpleCacheClient.GetBatchAsync(cacheName, keys); | ||
| if (response is CacheGetBatchResponse.Error badResponse) { | ||
| Assert.Equal(MomentoErrorCode.TIMEOUT_ERROR, badResponse.ErrorCode); | ||
| } |
| { | ||
| var ret = new List<CacheGetStatus>(); | ||
| foreach (CacheGetResponse response in Responses) { | ||
| if (response is CacheGetResponse.Hit hitResponse) { |
There was a problem hiding this comment.
These look better/safer now, so thanks for changing them.
I'm still feeling iffy about this part of the API as a whole, so I think we should revisit it and have a convo about it later this week, but I think it's okay for now for this PR.
There was a problem hiding this comment.
Agreed, the batch stuff needs some love and we should take a closer look when time allows.
| public class Success : CacheGetResponse | ||
| { | ||
| public CacheGetStatus Status { get; } | ||
| protected readonly ByteString? value; |
There was a problem hiding this comment.
That commit looks a great improvement but the field is still nullable here... it shouldn't need to be anymore, should it?
|
|
||
| // _GetResponse serverResponseBadRequest = new _GetResponse() { Result = ECacheResult.Invalid }; | ||
| // _ = Assert.Throws<InternalServerException>(() => new CacheGetResponse.Success(serverResponseBadRequest)); |
There was a problem hiding this comment.
Yes, those are obsolete due to class restructuring. The response subtypes are tested in the integration tests, so we're not losing coverage.
|
Great callout on |
There was a problem hiding this comment.
cool, thanks. I think we should merge this as-is because it's a good step forward, but would like to circle back quickly and see if the enum is still necessary at all. It feels like the only thing that is currently forcing us to keep it is that batch api, and if that's true, we should talk about options for getting rid of that bit, because having the enum here feels inconsistent since we got rid of it in the other places.
| public class Hit : CacheGetResponse | ||
| { | ||
| public CacheGetStatus Status { get => CacheGetStatus.HIT; } | ||
| protected readonly ByteString value; |
There was a problem hiding this comment.
cool, thanks. I think we should merge this as-is because it's a good step forward, but would like to circle back quickly and see if the enum is still necessary at all. It feels like the only thing that is currently forcing us to keep it is that batch api, and if that's true, we should talk about options for getting rid of that bit, because having the enum here feels inconsistent since we got rid of it in the other places.
There was a problem hiding this comment.
That sounds good. The enum shouldn't be necessary but removing it altogether as part of this effort seemed risky.
No description provided.