fix: Lcs response parse

[jabolina]

Originally, the LCS response parsing depended on the order of the returned map. The len field updates when adding the values to the matches values. Just a simple fix to verify first which key was read in the #set(ByteBuffer) method.

[tishun]

Hey @jabolina ,

do you mind extending the StringCommandIntegrationTests to showcase the issue you are fixing?

[jabolina]

@tishun, I've extended the class but didn't push yet. Which server version does the CI run with? So I can check the tests. Depending on the version, the tests covering LCS aren't running anymore ☹️ since they require STRALGO to exist.

I've expanded the unit test, too. It should make the issue more clear. The idea is, with the payload (I added padding to make it easier to read):

%2
    $7 matches
        *1
            *3
                *2
                    :4
                    :7
                *2
                    :5
                    :8
                :4

    $3 len
        :6

Results in the response:

{len=6, matches=[{a={start=4, end=7}, b={start=5, end=8}, matchLen=4}]}

If the order of keys changes, len and then matches objects:

%2
    $3 len
        :6

    $7 matches
...

The result is:

{len=4, matches=[{a={start=6, end=4}, b={start=7, end=5}, matchLen=0}]}

Which is not correct anymore.

IIUC, the spec does not make assumptions about the order of elements in the map. It could receive the keys in any order. However, I believe Redis always returns in the same order, first matches and then len objects. This PR would make it more resilient to any changes/assumptions. The CLI utility shows the same result in any order.

It might be tricky (or impossible) to reproduce this change in the order of objects in the integration tests. However, the unit test should cover it.

[tishun]

@tishun, I've extended the class but didn't push yet. Which server version does the CI run with? So I can check the tests. Depending on the version, the tests covering LCS aren't running anymore ☹️ since they require STRALGO to exist.

The pipeline uses latest Redis CE right now, which you can run locally if you execute

make start

If successful the command will deploy all the required servers and then you can run any of the integration tests.

[tishun]

IIUC, the spec does not make assumptions about the order of elements in the map. It could receive the keys in any order. However, I believe Redis always returns in the same order, first matches and then len objects. This PR would make it more resilient to any changes/assumptions. The CLI utility shows the same result in any order.

Oh, I see, I understand now - this is not an actual issue, but more of a guard in case the server starts to return results in another manner. In this case you are unlikely to be able to force the server to return the results in another manner, so an integration test is not necessary.

Let me spend some more time thinking on this. TBH the StringMatchResultOutput class seems extremely fragile as a whole right now

[jabolina]

@tishun, yes. I'll push what I have in a second commit.

[jabolina]

I added a second commit to the PR. It re-works some of the StringMatchResultOutput a little and registers into the OutputRegistry. This change removes the boolean in the constructor and makes it possible to use the StringMatchResultOutput as a valid return type in the CommandFactory.

Now, not much of my liking. I added some integration tests when the server has the LCS command. I guess the STRALGO tests weren't running for some time. TBH they might not work, as it uses keys without populating the server first. The problem is the CommandFactory because Lettuce doesn't implement the direct LCS, only the STRALGO LCS.

[tishun]

I added a second commit to the PR. It re-works some of the StringMatchResultOutput a little and registers into the OutputRegistry. This change removes the boolean in the constructor and makes it possible to use the StringMatchResultOutput as a valid return type in the CommandFactory.

Now, not much of my liking. I added some integration tests when the server has the LCS command. I guess the STRALGO tests weren't running for some time. TBH they might not work, as it uses keys without populating the server first. The problem is the CommandFactory because Lettuce doesn't implement the direct LCS, only the STRALGO LCS.

Oh oh oh I got the whole picture now.

Okay, so let's reset the conversation, apologies for my lag. To state the obvious :

• we only have STRALGO implemented in Lettuce
• STRALGO was replaced by LCS with Redis 7.0
• Tests were not running since then
• We want to address the concerns in the StringMatchResultOutput

IMHO we should approach this in another way than your suggestion. The biggest problem is that we can't really use LCS right now. I've created #2987 to address the missing command. We could obviously send the LCS using the Custom commands (similar to how you've done it in the integration test) but this is not optimal in terms of connections. Also IMHO we should not spend time future proofing the STRALGO since it is already removed.

Instead we should implement the new command and - if we are to reuse the StringMatchResultOutput - we can then address the solution accordingly.

BTW to test against an older version you could modify the Makefile on line 9 and change REDIS ?= unstable to REDIS ?= 6.2.13

[jabolina]

Yeah, right on point 🎯

This approach in steps makes more sense. We can close this PR and handle the issues in increments. If any of the changes make sense to be pulled, the git history remains. Please, let me know if any help is needed.

Let me try running the tests.

Edit: The tests work fine because they don't operate over keys! The whole point is that STRALGO operates over the user's input. Even though the tests weren't running, they were fine!

[tishun]

Yeah, right on point 🎯

This approach in steps makes more sense. We can close this PR and handle the issues in increments. If any of the changes make sense to be pulled, the git history remains. Please, let me know if any help is needed.

I think the code you added makes sense until we have LCS implemented.
It allows you to build the LCS command as a custom command and this is a good temporary workaround.
It also fixes the (possible) problems with the way the result is parsed.

When we implement the LCS command we can revise the code again if we want/need to.

Edit: The tests work fine because they don't operate over keys! The whole point is that STRALGO operates over the user's input. Even though the tests weren't running, they were fine!

Thanks for checking that. It would be good to know we are not regressing anything already working.