improve membersURI#812
Conversation
bonustrack
left a comment
There was a problem hiding this comment.
I'll review this PR, don't merge yet
Co-authored-by: Less <bonustrack@users.noreply.github.com>
Co-authored-by: Less <bonustrack@users.noreply.github.com>
| (SELECT DISTINCT voter AS address | ||
| FROM votes | ||
| WHERE space = ?) |
There was a problem hiding this comment.
I have placed the limit to the Outer Query, using the param, pageSize.
Placing a limit on the inner query would mess up the pagination, wouldn't it?
There was a problem hiding this comment.
Yes but this is taking a lot of time, especially for spaces with a lot of votes, we don't need to read all the distinct voters here, we just need 500 of them,
you can remove pagination from the outer query
Also for admins, moderators and members -> all this data is already available on space object, we don't need to read them from database again,
There was a problem hiding this comment.
We made an improvement on this, now the with cursor filter, if cursor is present, admins, moderators, and members are excluded from the distinct voter list. Also regardless we are not querying admins, moderators and members.
Query now only queries the Votes Table hence eliminating the need for subqueries.
There was a problem hiding this comment.
how to use cursor now? you have an example request? i tried few ways but couldn't get it working
There was a problem hiding this comment.
A request should be something like /ens.eth/members?cursor=983434323 ( Cursor Example )
Could you share what message do you get?
Could we get on a call and debug this? As I don't have a way to see whats wrong, its taking longer to close this issue
There was a problem hiding this comment.
Looks like we have to pass address to cursor now, something like http://localhost:3000/api/eip4824/ens.eth/members?cursor=0xB8cb5Bbf2DdD552145336436b2A0FD490aBf88Ca works
|
Apart from above comments, everything looks to be working nicely 👍 |
Co-authored-by: Chaitanya <yourchaitu@gmail.com>
Co-authored-by: Chaitanya <yourchaitu@gmail.com>
|
@bonustrack Looks like we need your approval
|
| let exclusionClause = ''; | ||
| if (exclusionList.length > 0) { | ||
| const placeholders = exclusionList.map(() => '?').join(', '); | ||
| exclusionClause = `AND voter NOT IN (${placeholders})`; |
There was a problem hiding this comment.
Can't this be used to inject arbitrary SQL? I think placeholders should be added as params so it get parsed.
There was a problem hiding this comment.
I don't think that would be of concern here because
- Placeholders values are not user input values
- Placeholder values are either empty lists or space.admins, space.moderators, space.members
params.push(...exclusionList)This pushes the actual exclusion list values (the voters you want to exclude) into the params array, which is passed to the query execution.const results =await db.queryAsync(query, params);The database library takes care of escaping the values in params, preventing SQL injection.
There was a problem hiding this comment.
These values (admins, moderators, members) are actually inputs from space admin, even if we have a strict format, I would prefer if that query doesn't depend on validation of such user input. And exclusionList is parsed but not exclusionClause. If you can update that we can move forward with your PR.

Fixes #825
Changes proposed in this pull request: