Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Changes to outbound redis interfaces #1873

Merged
merged 8 commits into from
Oct 13, 2023
Merged

Changes to outbound redis interfaces #1873

merged 8 commits into from
Oct 13, 2023

Conversation

rylev
Copy link
Collaborator

@rylev rylev commented Oct 10, 2023

Part of #1866

The changes to the interface are:

  • Uses a connection oriented resource rather than free functions.
  • Updates the error type from just being error to 4 different error possibilities.
  • Moves the functions that returned to s64 to return u32 since they should never return anything by positive numbers
  • get returns result<option<list<u8>>, error> rather than result<list<u8>, error> returning ok(none) when the key does not exist - this mirrors how Redis's server API.

Base automatically changed from resources to main October 11, 2023 12:47
@rylev rylev marked this pull request as ready for review October 11, 2023 12:49
@rylev rylev requested review from dicej and lann October 11, 2023 12:50
wit/preview2/redis.wit Outdated Show resolved Hide resolved
wit/preview2/redis.wit Outdated Show resolved Hide resolved
wit/preview2/redis.wit Outdated Show resolved Hide resolved
wit/preview2/redis.wit Outdated Show resolved Hide resolved
@rylev
Copy link
Collaborator Author

rylev commented Oct 11, 2023

In #1866 I said we should change get to return option so that it would return ok(none) if the key is not set. However, the underlying redis crate returns an empty Vec if it sees nil. Should we continue to propagate that behavior or should we return option? The current behavior may be surprising to those familiar with the redis API.

@lann
Copy link
Collaborator

lann commented Oct 11, 2023

Looks like the underlying get can return any FromRedisValue, which could be an Option<Vec<u8>> to preserve the nil: https://docs.rs/redis/latest/src/redis/types.rs.html#1516

@rylev
Copy link
Collaborator Author

rylev commented Oct 11, 2023

@lann my question isn't how to make it work but rather what should the API be? Is it better to mimic the way the redis crate works or is it better to be more true to the underlying redis API?

@lann
Copy link
Collaborator

lann commented Oct 11, 2023

Unless I'm missing something, the underlying redis API supports both, depending on whether your call to get uses RV = Vec<u8> or RV = Option<Vec<u8>>. It is just our current implementation that (implicitly, via inference) uses RV = Vec<u8>.

I assume this is the trait method being called: https://docs.rs/redis/0.23.3/redis/trait.AsyncCommands.html#method.get

@lann
Copy link
Collaborator

lann commented Oct 11, 2023

Sorry, I misread/misinterpreted what you were saying.

I think get should either return none or an error (e.g. no-such-key). Probably would lean toward an option especially since this is a behavior change.

rylev added 7 commits October 11, 2023 18:28
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@rylev rylev merged commit 1c37552 into main Oct 13, 2023
@rylev rylev deleted the outbound-redis branch October 13, 2023 13:42
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants