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

PATCH for groups returns 500 when members are updated #731

Closed
antifree opened this issue Apr 17, 2024 · 7 comments
Closed

PATCH for groups returns 500 when members are updated #731

antifree opened this issue Apr 17, 2024 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@antifree
Copy link
Contributor

antifree commented Apr 17, 2024

Hello team!

I faced strange issue. For some groups on SCIM PATCH server returns 500 error.

In logs the following error is shown: 5 rows were copied to scim.SCIMRepresentationAttributeLst but only 2 were inserted.

The detailed stack trace:

MySqlConnector.MySqlException (0x80004005): 5 rows were copied to scim.SCIMRepresentationAttributeLst but only 2 were inserted.
at MySqlConnector.MySqlBulkCopy.WriteToServerAsync(IOBehavior ioBehavior, CancellationToken cancellationToken) in //src/MySqlConnector/MySqlBulkCopy.cs:line 347
at MySqlConnector.MySqlBulkCopy.WriteToServerAsync(DataTable dataTable, CancellationToken cancellationToken) in /
/src/MySqlConnector/MySqlBulkCopy.cs:line 148
at EFCore.BulkExtensions.SqlAdapters.MySql.MySqlAdapter.InsertAsync[T](DbContext context, Type type, IEnumerable1 entities, TableInfo tableInfo, Action1 progress, Boolean isAsync, CancellationToken cancellationToken)
at EFCore.BulkExtensions.SqlAdapters.MySql.MySqlAdapter.InsertAsync[T](DbContext context, Type type, IEnumerable1 entities, TableInfo tableInfo, Action1 progress, Boolean isAsync, CancellationToken cancellationToken)
at EFCore.BulkExtensions.SqlAdapters.MySql.MySqlAdapter.InsertAsync[T](DbContext context, Type type, IEnumerable1 entities, TableInfo tableInfo, Action1 progress, CancellationToken cancellationToken)
at EFCore.BulkExtensions.SqlBulkOperation.InsertAsync[T](DbContext context, Type type, IEnumerable1 entities, TableInfo tableInfo, Action1 progress, CancellationToken cancellationToken)
at EFCore.BulkExtensions.DbContextBulkTransaction.ExecuteAsync[T](DbContext context, Type type, IEnumerable1 entities, OperationType operationType, BulkConfig bulkConfig, Action1 progress, CancellationToken cancellationToken)
at SimpleIdServer.Scim.Commands.Handlers.PatchRepresentationCommandHandler.UpdateRepresentation(SCIMRepresentation existingRepresentation, PatchRepresentationCommand patchRepresentationCommand, SCIMSchema schema)
at SimpleIdServer.Scim.Commands.Handlers.PatchRepresentationCommandHandler.UpdateRepresentation(SCIMRepresentation existingRepresentation, PatchRepresentationCommand patchRepresentationCommand, SCIMSchema schema)

SCIM Provider: Azure AD
Database: MySQL 8.0 (AWS Aurora MySQL to be specific)
Entity Framework Provider: Pomelo.EntityFrameworkCore.MySql
SimpleIdServer Version: 4.0.8

Could you please take a look at it?

Please let me know, if more information should be provided.

@simpleidserver
Copy link
Owner

I quickly checked the Nuget Package EFCore.BulkExtensions, which is used by SimpleIdServer for bulk data insertion.
It appears to utilize the Nuget Package MySQLConnector.
According to Ticket Ticket #1145, the Nuget package lacks sufficient information to diagnose the data issue causing the errors.

In the EFSCIMRepresentationCommandRepository class, could you please update the methods BulkInsert, BulkDelete, and BulkUpdate to capture the error messages originating from the MySQLConnection?

The code should resemble something like this:

var connection = context.Database.GetDbConnection() as MySqlConnection;
connection.InfoMessage += (s, e) =>
{
	// use logging infrastructure of your choice
	foreach (var error in e.Errors)
		Console.WriteLine(error.Message);
};

If this is too challenging, could you monitor and provide the incoming HTTP request that triggers this issue? I will attempt to reproduce the problem.

@antifree
Copy link
Contributor Author

antifree commented Apr 18, 2024

Hi @simpleidserver !

Thanks a lot for the recommendation!

I spent some time previously to set up test environment local to reproduce the issue and I included library directly.

So the error it the following:

Cannot add or update a child row: a foreign key constraint fails (`scim`.`SCIMRepresentationAttributeLst`, CONSTRAINT `FK_SCIMRepresentationAttributeLst_SCIMRepresentationAttributeLs~` FOREIGN KEY (`ParentAttributeId`) REFERENCES `SCIMRepresentationAttribute)

I assume it is related to the order of bulk insert:

As you can see, the list of inserted attributes is the following:
image

It seems the groups attribute itself is the last in groups.* sub attributes set.

I think groups and members.type attributes are set successfully, but sub attributes of user's groups property fails because it tries to insert it before.

P.s.

var attributesToInsert = scimRepresentationAttributes.ToList();

@antifree
Copy link
Contributor Author

So, i've made this small hack to test my theory:

var attributesToInsert = scimRepresentationAttributes.OrderBy(x => x.ParentAttributeId).ToList();

It placed groups before related sub-properties.

And it worked.

If you could suggest me, where is better to fix ordering, I could prepare an MR with more correct fix

@simpleidserver simpleidserver self-assigned this Apr 18, 2024
@simpleidserver simpleidserver added the bug Something isn't working label Apr 18, 2024
@simpleidserver
Copy link
Owner

Thank you for the information :)

I'll take a look and fix this issue for the next release!

@simpleidserver
Copy link
Owner

Great!

You can submit the pull request with this fix :)

@antifree
Copy link
Contributor Author

antifree commented Apr 18, 2024

So, I think I found where it is better to make this fix.

I think in RepresentationReferenceSync.BuildScimRepresentationAttribute we should return parent not the last, but the first.

I can prepare MR a little bit later

antifree added a commit to antifree/SimpleIdServer that referenced this issue Apr 18, 2024
simpleidserver added a commit that referenced this issue Apr 18, 2024
fix(#731): attributes ordering on insert
@simpleidserver
Copy link
Owner

This issue is fixed in the version 5.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Status: Done
Development

No branches or pull requests

2 participants