Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initial implementation of local-only users #1784

Merged
merged 17 commits into from
Oct 13, 2022
Merged

initial implementation of local-only users #1784

merged 17 commits into from
Oct 13, 2022

Conversation

davepacheco
Copy link
Collaborator

@davepacheco davepacheco commented Oct 5, 2022

This change adds APIs to create local users as described in RFD 321.

@@ -15,6 +15,7 @@ pub struct SiloUser {
#[diesel(embed)]
identity: SiloUserIdentity,

pub time_deleted: Option<chrono::DateTime<chrono::Utc>>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change deserves comment for future reference (but feel free to ignore this): this column was already part of the silo_user table and it's also in the definition of silo_user in schema.rs. It's not new in the database schema. One might think this field in this struct is already present via the identity field at L16 above, but that's only true for things that derive Resource. This derives Asset, which don't automatically get a time_deleted field. So we have to explicitly add it here.

The failure mode for not having this was that when I went to use check_if_exists()/execute_and_check(), I got this error:

[2022-10-07T01:42:14.308056515Z]  INFO: 4dcd44fe-92f8-41b7-9709-d57a03b4c148/dropshot_external/13170 on ivanova: request completed (req_id=c8658d5d-2708-4ab3-b4b2-1146851295d8, method=DELETE, remote_addr=127.0.0.1:56592, local_addr=127.0.0.1:38195, error_message_external="Internal Server Error", response_code=500)
    uri: /system/silos/local-only/identity-providers/local/users/id/ec8e5966-53ff-4866-86e8-d8db68783dfb
    --
    error_message_internal: Unknown diesel error accessing SiloUser ById(ec8e5966-53ff-4866-86e8-d8db68783dfb): Unexpected null for non-null column

The reason is that check_if_exists() generates SQL that selects all of the columns that Diesel knows about. Then execute_and_check() attempts to deserialize those columns into this type (SiloUser). I believe it does not use SiloUser::as_select() (as other things do), but by explicitly providing the Q generic argument to get_result_async(). I believe that causes the fields to be deserialized in whatever order they appear. There was a mismatch here because this field was missing. The particular error about a null value happened because the database was providing a null value for time_deleted, but it was being deserialized into a non-nullable field silo_id.

@davepacheco davepacheco requested a review from jmpesp October 7, 2022 03:47
@davepacheco davepacheco marked this pull request as ready for review October 7, 2022 03:47
@davepacheco
Copy link
Collaborator Author

This change is ready for review. @jmpesp, mind taking a look?

nexus/src/external_api/http_entrypoints.rs Outdated Show resolved Hide resolved
nexus/src/external_api/http_entrypoints.rs Outdated Show resolved Hide resolved
nexus/src/external_api/http_entrypoints.rs Outdated Show resolved Hide resolved
nexus/src/external_api/http_entrypoints.rs Outdated Show resolved Hide resolved
nexus/src/app/silo.rs Outdated Show resolved Hide resolved
nexus/src/app/silo.rs Outdated Show resolved Hide resolved
nexus/src/app/silo.rs Outdated Show resolved Hide resolved
nexus/src/app/silo.rs Outdated Show resolved Hide resolved
nexus/src/app/silo.rs Outdated Show resolved Hide resolved
nexus/src/app/silo.rs Outdated Show resolved Hide resolved
@davepacheco davepacheco merged commit fe04dbe into main Oct 13, 2022
@davepacheco davepacheco deleted the local-user-crud branch October 13, 2022 18:20
@davepacheco davepacheco mentioned this pull request Oct 13, 2022
69 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants