Lisk-sdk: Fix rank parameter of delegates API endpoint

[Tschakki]

Expected behavior

By making an API request to the delegates endpoint, with provided rank param, one should get a response that contains information about the delegate with the specified rank.
Also, I think it should accept numbers, not strings as values and could therefore be refactored.

Actual behavior

The response contains all delegates.
The param expects a string.

Steps to reproduce

Make API call
/api/delegates?rank="5"

Which version(s) does this affect? (Environment, OS, etc...)

1.0.0

[4miners]

@Tschakki Any practical use cases for that endpoint?

[mauliksoneji]

Hi @Tschakki and @4miners, If there need to be changes made, can you please assign this task to me? I can look into this.

[Tschakki]

@4miners ok alternative is just to get rid of that rank param...
One can query the delegates sorted by rank, what is enough for use cases like displaying rankings imho.
But I don't have a strong opinion in wether fixing or removing it.

[MaciejBaj]

@Mastermaulik are you still willing to do the task?

[mauliksoneji]

Yes, I can work on this issue @MaciejBaj.

As far as I understand, currently there is no rank parameter that we take in the /api/delegates endpoint. We need to change this endpoint to take in rank parameter and return the delegates having the same rank parameter.

The type of rank field is number, so it would look something like: /api/delegates?rank=5.
Is this what is the expected behaviour?

Like @Tschakki said, one can also query api/delegates?orderBy=rank:asc
To get the delegates in the increasing order of rank.

[Tschakki]

Thx for looking into it @Mastermaulik !

I think the most convenient will be to simply remove it, as this endpoint is not really needed... The same can be achieved with the alternative request api/delegates?orderBy=rank:asc, as you mentioned.

This is just a recommendation, if you want to fix it or already started, go ahead :)

[diego-G]

@Tschakki after reading all your comments, IMHO I think this parameter should be kept. I don't see how it can bother anyone. The use case will arrive with the time.
By itself, I see much more straightforward to request rank=101 than offset=100&limit=1&sort=rank:asc. That's quite clear.
@Mastermaulik if you don't mind, I gotta close the current PR. Would you be eager to fix the endpoint as it was intended at the beginning?

[mauliksoneji]

Hi @diego-G , I have had an eye surgery recently and am advised to take rest till monday, Please give me time till monday, I will work on the issue then.

[MaciejBaj]

@nazarhussain please suggest the proper solution.

[nazarhussain]

@Tschakki I reviewed the discussion as well the relevant code. Our db layer is written in a way to to accept filters fo only persisted database column. While rank is a computed column, so we have to refactor a lot of stuff to make that filter work.

@diego-G Providing composite filter is what we should do, but in current scope. Currently its get more complicated. e.g. If some one request rank=23&offset=3, so logically its an invalid request, as both params can be used together. So we have to an extensive request validation layer to send proper error messages.

For now its seems feasible to just remove the rank filter. Use can still get a specific rank account by providing multiple filter options e.g. offset=100&limit=1&sort=rank:asc. @MaciejBaj

[Tschakki]

@nazarhussain thanks for clarification!
I reopened @Mastermaulik 's PR, looks good from my side :)