mirror of
https://github.com/lldap/lldap.git
synced 2026-04-25 08:15:52 +03:00
[GH-ISSUE #821] Attributes are not sorted, users are not sorted, or previous user didn't consume all the attributes #294
Labels
No labels
backend
blocked
bug
cleanup
dependencies
docker
documentation
duplicate
enhancement
enhancement
frontend
github_actions
good first issue
help wanted
help wanted
integration
invalid
ldap
pull-request
question
rust
rust
tests
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/lldap-lldap#294
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @dvv on GitHub (Jan 29, 2024).
Original GitHub issue: https://github.com/lldap/lldap/issues/821
Describe the bug
Panic "Attributes are not sorted, users are not sorted, or previous user didn't consume all the attributes" after migration.
To Reproduce
Steps to reproduce the behavior:
Expected behavior
Server just works.
Is there a way to repack the database (postgresql) to fix the problem?
@nitnelave commented on GitHub (Jan 30, 2024):
Hmm, I can't reproduce the panic. Could you post a sql dump of the DB, or send it to me privately on Discord? Don't worry, I won't be able to get the passwords.
@dvv commented on GitHub (Jan 30, 2024):
@nitnelave sorry, no, I don't own it.
Indeed I saw no sorting in migration tool hence user ids come assorted from the source LDAP.
At first I tried to sort them:
but the error persisted.
Then I commented out the assertion:
and it worked.
Still I can not use it: I cured symptom but indeed I broke the logic. And I do not understand the logic.
@nitnelave commented on GitHub (Jan 30, 2024):
The ordering comes from the SQL query, not the migration: the query is ordered by user id, and so are the attributes. Is there a debug mode in PostgreSQL that prints out the queries? Otherwise, you can run LLDAP with RUST_LOG=debug, I think that should print a lot of things, including the SQL queries used.
As for the logic, we're iterating over the users and the attributes vectors, associating every run of attributes with the same user id with the corresponding user. That relies on both users and attributes to be sorted by user id.
@nitnelave commented on GitHub (Jan 30, 2024):
Just to confirm, you used the migration tool provided by LLDAP, correct? Can you confirm that all the user IDs are lowercase in the DB? Something like "SELECT user_id FROM users;" (or select I'd maybe)
@dvv commented on GitHub (Jan 30, 2024):
Yes, exactly.
Lowercased:
@dvv commented on GitHub (Jan 30, 2024):
I see when listing users in web ui:
The only quirk I see is an awful monstrous
... IN (...)clause. But I can not find the query source. Does it come from some ORM? Hmm. What if I'd have much more than 476 users?IMO such queries should read
select ... from ... where foo = any($1)passing the array to the parameter.@nitnelave commented on GitHub (Jan 30, 2024):
Ah, yeah, LLDAP was not built with "big" servers in mind, more for self-hosted homelabs. But still, 476 users is not that much, it should work.
One limitation that we have is that we support only the subset of SQL common to PG, MySQL and SQLite. That often leads us to write less-than-ideal queries. I'll see if it's possible to pass an array, but I didn't think that an online table would be a problem.
@dvv commented on GitHub (Jan 30, 2024):
I'm struggling with this:
Can you point out what's wrong there?
@nitnelave commented on GitHub (Jan 30, 2024):
I'm not in front of the computer, but you can try with
user_ids.collect::<Vec<_>>()@nitnelave commented on GitHub (Jan 30, 2024):
Got it to compile with:
But then we hit
Sqlite doesn't support array arguments.Can you try https://github.com/lldap/lldap/tree/filters instead? I repeat the query above with the same filter as a filter subquery. It's a bit ugly, but it should work (and it passes the tests).
@dvv commented on GitHub (Jan 31, 2024):
"filters" downs at the same assert
@dvv commented on GitHub (Jan 31, 2024):
Well,
but
@nitnelave commented on GitHub (Jan 31, 2024):
Huh, I didn't think that string comparison would be a sticking point. And
if you remove the assert, are things broken? Both lists should be in the
same order, even if it's not the Rust order.
On Wed, 31 Jan 2024, 05:24 Vladimir Dronnikov, @.***>
wrote:
@nitnelave commented on GitHub (Jan 31, 2024):
Ah, no, itertools is going to break since it'll advance the wrong one...
On Wed, 31 Jan 2024, 07:04 Valentin Tolmer, @.***> wrote:
@dvv commented on GitHub (Jan 31, 2024):
I fix the issue altering the collation of the database.
This one may be closed.
Will you pull filters' query so that no long
IN (...)live in the main branch?@nitnelave commented on GitHub (Feb 2, 2024):
@dvv sure. Can you give me the SQL command you ran to update the collation? I'd like to include that in the migration scripts.
@nitnelave commented on GitHub (Feb 2, 2024):
On second thought, it shouldn't be necessary: the order should be the same, so just removing the assert should be enough.
@dvv commented on GitHub (Feb 2, 2024):
@nitnelave first I tried https://stackoverflow.com/a/61595309
but then just recreated database as follows:
@dvv commented on GitHub (Feb 2, 2024):
Right, I believe we may rely on database sort order -- be it this or that -- the point is to just compact related records, imo.
@nitnelave commented on GitHub (Feb 27, 2024):
On third thought, I think the itertools utility used for grouping uses the ordering, so we have to re-sort.
@nitnelave commented on GitHub (Oct 30, 2024):
I think this doesn't apply anymore. Closing