simpleidserver / SimpleIdServer

OpenID, OAuth 2.0, SCIM2.0, UMA2.0, FAPI, CIBA & OPENBANKING Framework for ASP.NET Core
https://simpleidserver.com/
Apache License 2.0
686 stars 90 forks source link

Question: Scim representation in events now contains only changes not the whole representation #616

Closed fbauer2 closed 7 months ago

fbauer2 commented 8 months ago

Hello,

First of all thank you very much for the work on this project!

I was using version 3.0.4 and in events the received representation was the whole one (with all its attributes), I just upgraded to 4.0.5-rc1 and noticed that now events do not contain the whole representation anymore, but only what was received in input (so not all attributes in case of patch for example).

Is it the new expected behaviour or a bug?

Thanks very much.

simpleidserver commented 7 months ago

Hello,

Firstly, I apologize for my delayed response :)

It's true that the complete representation is not returned in the Integration Event when HTTP PUT or HTTP PATCH operations are executed, and this is due to performance issues. In fact, we have a use case where a group can contain more than 1 million users. Instead of loading the entire group in memory, we only return the modifications made to it. Thanks to this approach, we avoid performance problems.

KR,

SID

fbauer2 commented 7 months ago

Hello, no problem, thanks very much for your answer, this fully makes sense. This was to validate if I should keep what I did (ie retrieve the representation by myself in the event consumer, as I need the full picture to do some processing). Cheers.

fbauer2 commented 7 months ago

Hello again,

In fact I just noticed this is also the case for DELETE, which is an issue in my scenario because I cannot retrieve the representation anymore (as it is already deleted when the event is sent). Would it be acceptable to put again the whole representation in the removed event? Potentially with a setting so that it is configurable?

In DeleteRepresentationCommandHandler attributes are already retrieved so if I understood it well the needed change would be to add result.FlatAttributes = attributes; ? (I tried it locally and it seems ok but as I am not fully familiar with the whole code base I am not fully sure this is the best way to do it)

Thanks very much.

simpleidserver commented 7 months ago

Indeed, I am going to make some modifications to the SCIM project to provide developers with the option to choose whether to return the entire representation in the event or not.

I moved the ticket for the next release 4.0.6 :)

fbauer2 commented 7 months ago

Thank you very much!

simpleidserver commented 7 months ago

I made some changes in the master branch, to return the whole representation in the RepresentationRemovedEvent event.

fbauer2 commented 7 months ago

Thank you, I tested it successfully from master.

As an optimization shouldn't we remove in the handler : var attributes = await _scimRepresentationCommandRepository.FindAttributes(representation.Id, CancellationToken.None); and get the attributes directly from the representation instead?

Do you have the possibility to create a 4.0.6-rc1 release? Thanks very much

simpleidserver commented 7 months ago

Indeed, the implementation can be simpler. I made some modifications as you suggested to use the attributes coming from _scimRepresentationCommandRepository.FindAttribute.

The NuGet package 4.0.6-rc1 is available in the NuGet feed !

https://www.nuget.org/packages/SimpleIdServer.Scim/4.0.6-rc1

fbauer2 commented 7 months ago

Thank you again for your reactivity, I validated the change with 4.0.6-rc1 !