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

Server api #529

Closed
wants to merge 11 commits into from
Closed

Server api #529

wants to merge 11 commits into from

Conversation

hcwilhelm
Copy link
Contributor

Please ignore ApiSpec and zio.package. I will fix them later when I am ready with the PR

@hcwilhelm hcwilhelm mentioned this pull request Jan 29, 2022
65 tasks
@hcwilhelm
Copy link
Contributor Author

@mijicd were you able to take a look at this PR? I would like to know if I am on the right track before putting too much effort into it.

Copy link
Member

@mijicd mijicd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass done, looks great. I'll do a more thorough review once it's ready. Please specify the PR scope once it's ready for review, and don't forget to reference the issue :).

Comment on lines 52 to 67
case object On extends Rule
case object Off extends Rule
case class KeyPattern(pattern: String) extends Rule
case object ResetKeys extends Rule
case class ChannelPattern(pattern: String) extends Rule
case object RestChannels extends Rule
case class AddCommand(command: String, subcommand: Option[String] = None) extends Rule
case class RemoveCommand(command: String) extends Rule
case class AddCategory(category: String) extends Rule
case class RemoveCategory(category: String) extends Rule
case object NoPass extends Rule
case class AddTextPassword(pw: String) extends Rule
case class RemoveTextPassword(pw: String) extends Rule
case class AddHashedPassword(hash: String) extends Rule
case class RemoveHashedPassword(hash: String) extends Rule
case object Reset extends Rule
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's seal the case classes.

Comment on lines 69 to 72
val AllKeys = KeyPattern("*")
val AllChannels = ChannelPattern("*")
val AllCommands = AddCommand("all")
val NoCommands = RemoveCommand("all")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make them final.

* @return
* a list of categories
*/
final def aclCat(): ZIO[RedisExecutor, RedisError, Chunk[String]] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove (). The same applies to all other no-arg functions.

case RespValue.Array(values) =>
val flags = ChunkOutput(MultiStringOutput).unsafeDecode(values(1))
val passwords = ChunkOutput(MultiStringOutput).unsafeDecode(values(3))
val commnads = MultiStringOutput.unsafeDecode(values(5))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a typo.

Comment on lines 76 to 77
flags: List[String],
passwords: List[String],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use Chunk wherever is possible.

),
suite("UserEntryOutput") (
testM("read user entry successfully") {
val userEntryRaw = "user default on nopass ~* &* +@all"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd define it in for comprehension, lifted in UIO. The same applies to other occurrences.

@hcwilhelm
Copy link
Contributor Author

First pass done, looks great. I'll do a more thorough review once it's ready. Please specify the PR scope once it's ready for review, and don't forget to reference the issue :).

@mijicd Thanks for your feedback I will address all your comments and continue implementing the server API commands

*/
final def aclLogReset: ZIO[RedisExecutor, RedisError, Unit] = {
val command = RedisCommand(AclLog, StringInput, UnitOutput)
command.run("RESET")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better use it like a separate command. Doesn't it work?

AclLogReset

@@ -787,4 +787,76 @@ object Output {
case other => throw ProtocolError(s"$other isn't an integer >= -1")
}
}

case object UserinfoOutput extends Output[UserInfo] {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
case object UserinfoOutput extends Output[UserInfo] {
case object UserInfoOutput extends Output[UserInfo] {

flags: Chunk[String],
firstKey: Int,
lastKey: Int,
step: Int,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, We have to stick with Long type everywhere, cause Redis integer is 64 bit in most cases, according to documentation Resp Integer

@hcwilhelm hcwilhelm closed this Sep 6, 2023
@hcwilhelm hcwilhelm deleted the server_api branch September 6, 2023 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants