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

[Host.HybridMessageSerialzier] Resolve HybridMessageSerializer dependencies from IServiceProvider #239

Merged

Conversation

EtherZa
Copy link
Contributor

@EtherZa EtherZa commented Apr 4, 2024

#238 PR to allow for serializers to be added and then resolved by the hybrid message serializer.

By resolving the specialized serializers from the DI container, registration of those serializers can be performed via their included builders. This will then allow for the expected dependency resolution path to be followed for each of the specialized serializers ie. JsonMessageSerializer can use a supplied JsonSerializerOptions instance or resolve one from the DI container directly

Caveat:
The HybridMessageSerializer must be registered last so that it is able to remove previous IMessageSerializer registrations in order to make itself the designated serializer.

    services
        .AddSlimMessageBus(mbb =>
        {
            mbb
                // add required serializers to the DI container first
                .AddAvroSerializer()
                .AddGoogleProtobufSerializer()
                .AddJsonSerializer()
                
                // Add hybrid serializer last so that it can update DI container and set itself as the default
                .AddHybridSerializer<JsonMessageSerializer>(
                    o => {

                        // Message1, Message2 => AvroSerializer
                        // Message3 => GoogleProtobufMessageSerializer
                        // all other messages by JsonMessageSerializer

                        o.Add<AvroSerializer>(typeof(Message1), typeof(Message2));
                        o.Add<GoogleProtobufMessageSerializer>(typeof(Message3));
                    })
                ...
        } 

@EtherZa EtherZa force-pushed the feature/hybrid-serializer-dependency-injection branch from 9361734 to f29f09e Compare April 4, 2024 04:31
@EtherZa
Copy link
Contributor Author

EtherZa commented Apr 4, 2024

Removed some syntactic sugar that SonarCloud wasn't a fan of.

@zarusz zarusz self-requested a review April 4, 2024 10:37
@zarusz zarusz added this to the 2.3.0 milestone Apr 4, 2024
Copy link
Owner

@zarusz zarusz left a comment

Choose a reason for hiding this comment

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

@EtherZa have a look at a few minor things.

I am also wondering if there is a way we could not require hybrid to be configured as the last (while ensuring backward compatibility with the overall serializer configuration experience).

Looks good overall!

@zarusz zarusz assigned zarusz and EtherZa and unassigned zarusz Apr 4, 2024
@EtherZa EtherZa force-pushed the feature/hybrid-serializer-dependency-injection branch 2 times, most recently from d5ecb88 to d0e60b1 Compare April 5, 2024 09:00
@EtherZa
Copy link
Contributor Author

EtherZa commented Apr 5, 2024

@EtherZa have a look at a few minor things.

I am also wondering if there is a way we could not require hybrid to be configured as the last (while ensuring backward compatibility with the overall serializer configuration experience).

Looks good overall!

I have added a potential option that you may prefer in the issue.

@EtherZa EtherZa force-pushed the feature/hybrid-serializer-dependency-injection branch 3 times, most recently from cbbe35e to 807368f Compare April 6, 2024 15:01
@EtherZa EtherZa force-pushed the feature/hybrid-serializer-dependency-injection branch from 807368f to 4e7700b Compare April 8, 2024 05:20
…Provider

Signed-off-by: Richard Pringle <richardpringle@gmail.com>
@EtherZa EtherZa force-pushed the feature/hybrid-serializer-dependency-injection branch from 4e7700b to 2829d04 Compare April 8, 2024 05:22
@EtherZa EtherZa requested a review from zarusz April 8, 2024 05:24
Copy link

sonarqubecloud bot commented Apr 8, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@zarusz zarusz merged commit 30d9177 into zarusz:master Apr 8, 2024
2 checks passed
@zarusz
Copy link
Owner

zarusz commented Apr 8, 2024

@EtherZa thanks for your contribution looks good!

If you have a moment would be good to tweak these 4 sonar warnings (in another PR). Else I can look at them in a few days. Cheers.

@EtherZa EtherZa deleted the feature/hybrid-serializer-dependency-injection branch April 9, 2024 01:02
EtherZa added a commit to EtherZa/SlimMessageBus that referenced this pull request Apr 9, 2024
Signed-off-by: Richard Pringle <richardpringle@gmail.com>
@EtherZa
Copy link
Contributor Author

EtherZa commented Apr 9, 2024

@EtherZa thanks for your contribution looks good!

Fantastic!

If you have a moment would be good to tweak these 4 sonar warnings (in another PR). Else I can look at them in a few days. Cheers.

#243 fixes the issues.

Thanks for the support/ideas in getting it across the line.

zarusz pushed a commit that referenced this pull request Apr 9, 2024
Signed-off-by: Richard Pringle <richardpringle@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants