taoensso / carmine

Redis client + message queue for Clojure
https://www.taoensso.com/carmine
Eclipse Public License 1.0
1.16k stars 132 forks source link

xtrim parameters regression? #283

Closed kwisath closed 1 year ago

kwisath commented 1 year ago

On the commands.edn file of commit 5f9f6b299fae2188b6bf1238b4f03750f21c54df XTRIM is defined as follows

"XTRIM" {:fn-name "xtrim", 
:fn-docstring "Trims the stream to (approximately if '~' is passed) a certain size.\n\nXTRIM key trim\n\nAvailable since: 5.0.0.\n\nTime complexity: O(N), with N being the number of evicted entries. Constant times are very small however, since entries are organized in macro nodes containing multiple entries that can be released with a single deallocation.", 
:fn-params-fixed [key trim], 
:fn-params-more nil, 
:req-args-fixed ["XTRIM" key trim], 
:cluster-key-idx 1}

while the traditional way in which it was defined was

"XTRIM" {:fn-name "xtrim", 
:fn-docstring "Trims the stream to (approximately if '~' is passed) a certain size.\n\nXTRIM key MAXLEN [~] count\n\nAvailable since: 5.0.0.\n\nTime complexity: O(N), with N being the number of evicted entries. Constant times are very small however, since entries are organized in macro nodes containing multiple entries that can be released with a single deallocation.", 
:fn-params-fixed [key strategy], 
:fn-params-more [key strategy & args], 
:req-args-fixed ["XTRIM" key strategy], 
:cluster-key-idx 1}

Note that the new version has :fn-params-more nil which prevents xtrim from being called like (car/xtrim stream :MINID "~" last-ts). Per https://redis.io/commands/xtrim/ this type of invocation should be possible (and was totally possible and working as a charm in previous versions of the library due to the & args in :fn-params-more).

ptaoussanis commented 1 year ago

@kwisath Hi there, thanks for the report!

That's strange. Carmine's commands.edn file is produced directly from the official command spec.

If there's an incorrect entry in commands.edn, then it's either a regression in the official spec - or there's a bug in Carmine's command spec parser that appears to be triggering in only this case.

I'm planning to cut a major new Carmine release by the end of June, will take a closer look at this as part of that batched work.

As a temporary workaround in the meantime, you can always use Carmine's redis-call API to send arbitrary commands to Redis.

kwisath commented 1 year ago

Thanks @ptaoussanis ! I had a look at the official command spec you linked and the XTRIM appears with a format that I think may be confusing the spec parser (from a semantic point of view the command in the JSON file does not seem to be wrong)

 "arguments": [
            {
                "name": "key",
                "type": "key",
                "display_text": "key",
                "key_spec_index": 0
            },
            {
                "name": "trim",
                "type": "block",
                "arguments": [
                    {
                        "name": "strategy",
                        "type": "oneof",
                        "arguments": [
                            {
                                "name": "maxlen",
                                "type": "pure-token",
                                "display_text": "maxlen",
                                "token": "MAXLEN"
                            },
                            {
                                "name": "minid",
                                "type": "pure-token",
                                "display_text": "minid",
                                "token": "MINID",
                                "since": "6.2.0"
                            }
                        ]
                    },
                    {
                        "name": "operator",
                        "type": "oneof",
                        "optional": true,
                        "arguments": [
                            {
                                "name": "equal",
                                "type": "pure-token",
                                "display_text": "equal",
                                "token": "="
                            },
                            {
                                "name": "approximately",
                                "type": "pure-token",
                                "display_text": "approximately",
                                "token": "~"
                            }
                        ]
                    },
                    {
                        "name": "threshold",
                        "type": "string",
                        "display_text": "threshold"
                    },
                    {
                        "name": "count",
                        "type": "integer",
                        "display_text": "count",
                        "token": "LIMIT",
                        "since": "6.2.0",
                        "optional": true
                    }
                ]
            }

The trim argument in fact has nested arguments inside of it, and maybe the command spec parser is not prepared to deal with that.

ptaoussanis commented 1 year ago

The trim argument in fact has nested arguments inside of it, and maybe the command spec parser is not prepared to deal with that.

👍 PR welcome if you feel like taking a stab at updating the parser, otherwise I'll try take a look myself as part of the work for the upcoming v3.3.

Thanks again for pinging about this!

Cheers :-)

ptaoussanis commented 1 year ago

Closing, this will be addressed in forthcoming release.