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

Migrate the OpenID module to OpenIddict 5.0 #14808

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

kevinchalet
Copy link
Member

@kevinchalet kevinchalet commented Dec 2, 2023

OpenIddict 5.0 RTM is expected to ship later this month. This PR reacts to the breaking changes introduced by this new major by adding new store methods are updating the ones whose signature has changed.

(note: while OpenIddict 5.0 supports new features, this PR doesn't expose them via the OC GUI. If there's some interest from the community, it's certainly something we'll want to consider for another PR)

Edit: 5.0 RTM just shipped: https://kevinchalet.com/2023/12/18/openiddict-5-0-general-availability/

@kevinchalet kevinchalet added this to the 1.x milestone Dec 2, 2023
@kevinchalet kevinchalet self-assigned this Dec 2, 2023
= ImmutableDictionary.Create<string, string>();

/// <summary>
/// Gets or sets the client type associated with the current application.
/// </summary>
public string Type { get; set; }
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, we should rename this property to ClientType but it's not clear to me what would be the proper way to do it with YesSql /cc @sebastienros

Copy link
Contributor

Choose a reason for hiding this comment

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

@kevinchalet In the migrations, before any DDL commands I would create a new session and query the Documents table. Then in another session i would read the Applications by id (read from the previous query) and update the new ClientType with the Type value in Document. @sebastienros maybe YESSQL could provide a way to query the documents instead of writing a document query every time.
eg.

       public async Task<int> UpdateFrom1Async()
       {
           var sqlBuilder = new SqlBuilder(_session.Store.Configuration.TablePrefix, _session.Store.Configuration.SqlDialect);

           var tableName = _session.Store.Configuration.TableNameConvention.GetDocumentTable(Publication.Collection);
           var schema = _session.Store.Configuration.Schema;

           // Select Documents Ids
           sqlBuilder.Select();
           sqlBuilder.Selector(nameof(Document.Id));
           sqlBuilder.Table(tableName, null, schema);
           sqlBuilder.WhereAnd($" {sqlBuilder.FormatColumn(tableName, nameof(Document.Type), schema)} = @type");

           using (var documentsSession = _session.Store.CreateSession())
           {
               using (var connection = await documentsSession.CreateConnectionAsync())
               using (var migrateDocumentsSession = _session.Store.CreateSession())
               {
                   IEnumerable<int> documentIds;

                   documentIds = await connection.QueryAsync<int>(
                       sqlBuilder.ToSqlString(), new { type = $"{typeof(Publication).FullName}, {typeof(Publication).Assembly.GetName().Name}" });

                   sqlBuilder = new SqlBuilder(_session.Store.Configuration.TablePrefix, _session.Store.Configuration.SqlDialect);

                   // Select Documents
                   sqlBuilder.Select();
                   sqlBuilder.Selector("*");
                   sqlBuilder.Table(tableName, null, schema);
                   sqlBuilder.WhereAnd($" {sqlBuilder.FormatColumn(tableName, nameof(Document.Id), schema)} IN @ids");

                   IEnumerable<Document> documents;
                   foreach (IEnumerable<int> batch in documentIds.PagesOf(BATCH_SIZE))
                   {
                       var ids = batch.ToArray();

                       documents = await connection.QueryAsync<Document>(sqlBuilder.ToSqlString(), new { ids });

                       var publications = (await migrateDocumentsSession.GetAsync<Publication>(ids, PUBLICATION_COLLECTION)).GetEnumerator();

                       foreach (var document in documents)
                       {
                           var p = JObject.Parse(document.Content);
                           publications.MoveNext();
                           if (p.TryGetValue("DeployedUtc", out var jt))
                           {
                               publications.Current.CompletedUtc = jt.Value<DateTime?>();
                               migrateDocumentsSession.Save(publications.Current, PUBLICATION_COLLECTION);
                           };
                       }

                       await migrateDocumentsSession.SaveChangesAsync();
                   }
               }
           }

           SchemaBuilder.AlterIndexTable<PublicationIndex>(table => table
               .RenameColumn("PublishedUtc", nameof(PublicationIndex.CompletedUtc)),
                   collection: PUBLICATION_COLLECTION);

           return 2;

       }

Copy link
Contributor

Choose a reason for hiding this comment

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

And I think there is no need for two sessions/connections, if you get a connection after SaveChangesAsync()

Copy link
Member

Choose a reason for hiding this comment

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

@kevinchalet, you can mark Type as Obsolete and add a new property called ClientType. Setting Type can also set ClientType if ClientType is null.

@MichaelPetrinolis you may want to use await using to dispose the connection asynchronously. Also, that could should be call that query using ShellScope.AddDeferredTask to ensure that query is called after everything has been setup (specially during new setup)

Copy link
Contributor

Choose a reason for hiding this comment

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

@MikeAlhayek the Deferred Tasks execute after the migrations complete? If this is the case and the migration fails, subsequently the deferred task will also fail, as the DB won't have the new index table column name (if we also rename the column name 'Type' to 'ClientType' in the index). Maybe adding a DataMigration step that executes after schema migrations (considering the current schema version) would be handy.

Copy link
Member

Choose a reason for hiding this comment

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

Yes differed task will be executed at the end of the request. You can call it after the alter command. If the alter exceptionally failed, the next line will not be called.

Copy link
Contributor

Choose a reason for hiding this comment

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

@MikeAlhayek the issue with the deferred task is that the schema might get updated, but the data are not updated if an error during the update occurs. I think we need a two-step migration process, one to update the schema (existing) and one to run data migrations for the new Schema Version as deferred task.

Copy link
Member Author

Choose a reason for hiding this comment

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

@MichaelPetrinolis @MikeAlhayek thanks for your suggestions!

That said, it looks a bit risky and it's likely I won't have too much time to troubleshot any potential issue that may occur due to this change.

If anyone wants to submit a PR to change the name of the Type column to ClientType (once OpenIddict 5.0 ships next week), please feel free! 😃

/// Gets or sets the JSON Web Key Set associated with the current application.
/// </summary>
// TODO: change the property type to JsonWebKeySet after migrating to System.Text.Json.
public JObject JsonWebKeySet { get; set; }
Copy link
Member Author

Choose a reason for hiding this comment

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

I opted for JObject for consistency with other similar properties, but we'll be able to change it to JsonWebKeySet (an IdentityModel type that is now annotated with the System.Text.Json attributes since 7.0) as soon as YesSql supports System.Text.Json-based serialization/deserialization.

@kevinchalet kevinchalet modified the milestones: 1.x, 1.8 Dec 18, 2023
@kevinchalet kevinchalet merged commit 96c4905 into OrchardCMS:main Dec 18, 2023
3 checks passed
@kevinchalet kevinchalet deleted the openiddict_5.0 branch December 18, 2023 17:39
urbanit pushed a commit to urbanit/OrchardCore that referenced this pull request Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants