Attributes are not sorted, users are not sorted, or previous user didn't consume all the attributes #821
Labels
bug
Something isn't working
Comments
|
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. |
|
@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. --- a/migration-tool/src/main.rs
+++ b/migration-tool/src/main.rs
@@ -23,11 +23,12 @@ fn ask_generic_confirmation(name: &str, message: &str) -> Result<bool> {
fn get_users_to_add(users: &[User], existing_users: &[String]) -> Result<Option<Vec<User>>> {
let existing_users = HashSet::<&String>::from_iter(existing_users);
let num_found_users = users.len();
- let input_users: Vec<_> = users
+ let mut input_users: Vec<_> = users
.iter()
.filter(|u| !existing_users.contains(&u.user_input.id))
.map(User::clone)
.collect();
+ input_users.sort_by(|u1, u2| u1.user_input.id.cmp(&u2.user_input.id));
println!(
"Found {} users, of which {} new users: [\n {}\n]",
num_found_users,but the error persisted. Then I commented out the assertion: --- a/server/src/domain/sql_user_backend_handler.rs
+++ b/server/src/domain/sql_user_backend_handler.rs
@@ -145,11 +145,13 @@ impl UserListerBackendHandler for SqlBackendHandler {
let mut attributes_iter = attributes.into_iter().peekable();
use itertools::Itertools; // For take_while_ref
for user in users.iter_mut() {
+ /*
assert!(attributes_iter
.peek()
.map(|u| u.user_id >= user.user.user_id)
.unwrap_or(true),
"Attributes are not sorted, users are not sorted, or previous user didn't consume all the attributes");
+ */
user.user.attributes = attributes_iter
.take_while_ref(|u| u.user_id == user.user.user_id)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. |
|
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. |
|
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) |
|
Yes, exactly. Lowercased: lldap=# select user_id from users where user_id != lower(user_id);
user_id
---------
(0 rows) |
|
I see when listing users in web ui: The only quirk I see is an awful monstrous |
|
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. |
|
I'm struggling with this: let attributes = model::UserAttributes::find()
//.filter(model::UserAttributesColumn::UserId.is_in(user_ids)) // <-- this may choke
.filter(model::UserAttributesColumn::UserId.eq(Expr::cust_with_values("ANY(?)", [user_ids]))) <-- this does not compile
.order_by_asc(model::UserAttributesColumn::UserId)
.order_by_asc(model::UserAttributesColumn::AttributeName)
.all(&self.sql_pool)
.await?;Can you point out what's wrong there? |
|
I'm not in front of the computer, but you can try with |
|
Got it to compile with: let user_ids: Vec<_> = users.iter().map(|u| u.user.user_id.to_string()).collect();
let attributes = model::UserAttributes::find()
//.filter(model::UserAttributesColumn::UserId.eq(Func::cust(Any).arg(user_ids)))
.filter(Expr::cust_with_exprs(
"? = ANY(?)",
[
SimpleExpr::Column(model::UserAttributesColumn::UserId.into_column_ref()),
user_ids.into(),
],
))
.order_by_asc(model::UserAttributesColumn::UserId)
.order_by_asc(model::UserAttributesColumn::AttributeName)
.all(&self.sql_pool)
.await?;But then we hit 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). |
|
"filters" downs at the same assert |
|
Well, -- sqlite3
select 'test.tools' >= 'testsamba'; -- 0-- mysql
select 'test.tools' >= 'testsamba'; -- 0# rust
println!(">= {}", "test.tools" >= "testsamba"); // falsebut -- default postgres from docker
select 'test.tools' >= 'testsamba'; -- true |
|
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:
Well,
-- sqlite3
select 'test.tools' >= 'testsamba'; -- 0
-- mysql
select 'test.tools' >= 'testsamba'; -- 0
# rustprintln!(">= {}", "test.tools" >= "testsamba"); // false
but
-- postgres
select 'test.tools' >= 'testsamba'; -- true
—
Reply to this email directly, view it on GitHub
<#821 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGCPWLUWMDBBY7VCTDKQVTYRHBPHAVCNFSM6AAAAABCP3HKQSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJYGM2TKOBTGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Ah, no, itertools is going to break since it'll advance the wrong one...
…On Wed, 31 Jan 2024, 07:04 Valentin Tolmer, ***@***.***> wrote:
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:
> Well,
>
> -- sqlite3
> select 'test.tools' >= 'testsamba'; -- 0
>
> -- mysql
> select 'test.tools' >= 'testsamba'; -- 0
>
> # rustprintln!(">= {}", "test.tools" >= "testsamba"); // false
>
> but
>
> -- postgres
> select 'test.tools' >= 'testsamba'; -- true
>
> —
> Reply to this email directly, view it on GitHub
> <#821 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAGCPWLUWMDBBY7VCTDKQVTYRHBPHAVCNFSM6AAAAABCP3HKQSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJYGM2TKOBTGU>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
|
I fix the issue altering the collation of the database. |
|
@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. |
|
On second thought, it shouldn't be necessary: the order should be the same, so just removing the assert should be enough. |
|
@nitnelave first I tried https://stackoverflow.com/a/61595309 but then just recreated database as follows: |
|
Right, I believe we may rely on database sort order -- be it this or that -- the point is to just compact related records, imo. |
|
On third thought, I think the itertools utility used for grouping uses the ordering, so we have to re-sort. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
The text was updated successfully, but these errors were encountered: