Refactored serialization feature to be aligned with transport #3310

SzymonPobiega · 2016-01-11T13:35:09Z

Made feature class not abstract
Removed DI usages across the feature
Removed the intermediate ConfigureSerialization-inherited classes
Changed the API for SerializationDefinition

Associated Doco PR

~~Create a PR for SC Custom Checks plugin to not use built-in json serializer~~ @danielmarbach is already working on that
~~Create a PR for SC Heartbeat plugin to not use built-in json serializer~~ Fix added to release-1.0 branch: Particular/ServiceControl.Plugin.Nsb6.Heartbeat@93a892c

hmemcpy · 2016-01-11T13:36:53Z

Bye bye ConfigureSerialization :)

hmemcpy · 2016-01-12T09:02:24Z

@SzymonPobiega I'm working on the samples: Particular/docs.particular.net#1020
Do I need your PR for this? Are you changing the public API I need to implement external serialization features?

SzymonPobiega · 2016-01-12T09:05:58Z

The API for registering a custom serializer is different now (requires a SerializerDefinition class in addition to implementation of IMessageSerializer). Other stuff is the same. I'll wait for you to update the samples to current V6 and then I'll create my pull to accompany this PR.

hmemcpy · 2016-01-12T09:07:02Z

We're in a deadlock :D nice!
Will send a samples PR soon.

SzymonPobiega · 2016-01-27T08:27:25Z

Still a WIP, but @Particular/nservicebus-maintainers you can start reviewing. I might need to form a TF to take it further though. Currently will focus on doco PR and then spike another attempt to kill the message mapper.

danielmarbach · 2016-01-27T08:30:12Z

I think we should call YAGNI on this right now and focus on getting other things outside the door.

SzymonPobiega · 2016-01-27T08:33:50Z

@danielmarbach then I call it non-WIP and pause here. Consider this one done in terms of code, syncing with @hmemcpy with regards to doco updates now.

danielmarbach · 2016-01-27T08:37:57Z

Cheecky ;) Then I like it 💃 Can you rebase

SzymonPobiega · 2016-01-27T08:43:23Z

Aye aye, sir!

johnsimons · 2016-01-27T09:31:23Z

Why should this be done for v6.0?

SzymonPobiega · 2016-01-27T09:33:17Z

@johnsimons because I was bored on Koh Samet ;-)

SzymonPobiega · 2016-01-29T12:10:03Z

@Particular/nservicebus-maintainers can you please take a look at this one?

danielmarbach · 2016-01-29T12:55:47Z

src/NServiceBus.AcceptanceTests/EndpointTemplates/DefaultServer.cs

@@ -56,7 +57,7 @@ public DefaultServer(List<Type> typesToInclude)

            if (serializer != null)
            {
-                builder.UseSerialization(Type.GetType(serializer));
+                builder.UseSerialization((SerializationDefinition)Activator.CreateInstance(Type.GetType(serializer, true)));


This is ugly but I see there is no other way around because you want to deprecate

public static void UseSerialization(this NServiceBus.BusConfiguration config, System.Type serializerType)

I first thought we can repurpose the above method to:

public static void UseSerialization(this NServiceBus.BusConfiguration config, System.Type serializerDefinitionType)

But that would be strange

andreasohlund · 2016-01-29T13:10:41Z

@SzymonPobiega this will affect the downstream serializers right? (in that case we need to line up pull for them as well?)

danielmarbach · 2016-01-29T13:42:02Z

Yes at least this one

https://github.com/Particular/NServiceBus.Newtonsoft.Json

This rest is not officially owned by us

danielmarbach · 2016-01-29T13:42:13Z

I really like it!

mauroservienti · 2016-01-29T15:11:28Z

should we consider this in scope for V6? if yes, could you please ad the expected Tag: Core V6 label? cheers.

danielmarbach · 2016-01-29T15:15:06Z

src/NServiceBus.Core/Serialization/SerializationFeature.cs

+            context.Pipeline.Register("SerializeMessageConnector", new SerializeMessageConnector(defaultSerializer, messageMetadataRegistry), "Converts a logical message into a physical message");
+
+            context.Container.ConfigureComponent(_ => mapper, DependencyLifecycle.SingleInstance);
+            context.Container.ConfigureComponent(_ => messageMetadataRegistry, DependencyLifecycle.SingleInstance);


For backwards compat reasons should we add?

context.Container.ConfigureComponent(_ => defaultSerializer, DependencyLifecycle.SingleInstance);

// cc @Scooletz

danielmarbach · 2016-01-29T15:39:34Z

I say yes. I'll add the label

…ore's message serializer and there's no way TransportDefinition can access it.

SzymonPobiega · 2016-02-01T04:26:14Z

@andreasohlund @danielmarbach that downstream has already been updated: https://github.com/Particular/NServiceBus.Newtonsoft.Json/tree/update-v6

Scooletz · 2016-02-01T10:16:56Z

I had a discussion with @danielmarbach & @andreasohlund was summarized in Particular/NServiceBus.AzureStorageQueues#29. I'm going to ensure wire compatibility with tests on ASQ end and detect by ReadOnlySettings what kind of serializer is used in Core. Then provide either proper impl or throw a configuration exception.
This removes any requirements from this PR. I'll need only the name of the serializer in the settings.

SzymonPobiega · 2016-02-01T11:47:31Z

Yupi!

SzymonPobiega · 2016-02-02T07:05:28Z

Rabased, squashed and changed public configure to protected internal. If the ATTs pass I think it is ready to be merged.

danielmarbach · 2016-02-02T07:12:47Z

See my last comment about obsoletes

SzymonPobiega · 2016-02-02T07:22:53Z

@danielmarbach obsoletes added. I didn't have to rename because internal types are in the root namespace.

SzymonPobiega · 2016-02-02T12:46:55Z

Ping

Refactored serialization feature to be aligned with transport

SimonCropp · 2016-02-10T07:13:22Z

@danielmarbach @SzymonPobiega so this broke docs in a way that i dont know how to work around. specifically this sample http://docs.particular.net/samples/pipeline/multi-serializer/#code-walk-through-serialization-mapper can you let me know how i tweak that sample to get it working

CC @indualagarsamy

danielmarbach · 2016-02-10T07:16:11Z

Since this has an associated doco PR, it fulfilled all the necessary requirements to be able to get merge.

SimonCropp · 2016-02-10T07:18:38Z

@danielmarbach that PR did not touch the sample in question

danielmarbach · 2016-02-10T07:29:24Z

If @SzymonPobiega doesn't find time until tomorrow I can have s look after
tomorrow after my day off

On Wednesday, February 10, 2016, Simon Cropp notifications@github.com
wrote:

@danielmarbach https://github.com/danielmarbach that PR did not touch
the sample in question

—
Reply to this email directly or view it on GitHub
#3310 (comment)
.

SzymonPobiega · 2016-02-10T07:56:13Z

Looking at it now.

SimonCropp · 2016-02-10T07:56:55Z

@SzymonPobiega thanks

andreasohlund · 2016-02-10T12:25:09Z

Here is the proposed core fix #3444 (comment)

SzymonPobiega added the State: In Progress label Jan 11, 2016

SzymonPobiega self-assigned this Jan 11, 2016

SzymonPobiega added this to the 6.0.0 milestone Jan 11, 2016

SzymonPobiega force-pushed the refactor-serialization-config branch from 25b88df to 7ae3425 Compare January 27, 2016 08:32

SzymonPobiega changed the title ~~[WIP] Refactored serialization feature to be aligned with transport.~~ Refactored serialization feature to be aligned with transport. Jan 27, 2016

SzymonPobiega force-pushed the refactor-serialization-config branch from 7ae3425 to 8b96302 Compare January 27, 2016 08:41

andreasohlund mentioned this pull request Jan 28, 2016

Merge is blocked even though the [wip] prefix was removed sanemat/do-not-merge-wip-for-github#80

Closed

andreasohlund changed the title ~~Refactored serialization feature to be aligned with transport.~~ Refactored serialization feature to be aligned with transport Jan 28, 2016

danielmarbach reviewed Jan 29, 2016
View reviewed changes

danielmarbach added Tag: Core v6 labels Jan 29, 2016

SzymonPobiega referenced this pull request in Particular/NServiceBus.AzureStorageQueues Jan 29, 2016

Own JSON serialization for MessageWrapper as there's no need to use c…

0d64f1d

…ore's message serializer and there's no way TransportDefinition can access it.

SzymonPobiega force-pushed the refactor-serialization-config branch from 8b96302 to aac8ad7 Compare February 2, 2016 06:58

SzymonPobiega force-pushed the refactor-serialization-config branch from aac8ad7 to 9f98cec Compare February 2, 2016 07:22

Refactored serialization feature to be aligned with transport.

522594b

SzymonPobiega force-pushed the refactor-serialization-config branch from 9f98cec to 522594b Compare February 2, 2016 10:13

danielmarbach added a commit that referenced this pull request Feb 2, 2016

Merge pull request #3310 from Particular/refactor-serialization-config

2c142d5

Refactored serialization feature to be aligned with transport

danielmarbach merged commit 2c142d5 into develop Feb 2, 2016

danielmarbach added State: In Progress and removed State: In Progress labels Feb 2, 2016

danielmarbach deleted the refactor-serialization-config branch February 2, 2016 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored serialization feature to be aligned with transport #3310

Refactored serialization feature to be aligned with transport #3310

SzymonPobiega commented Jan 11, 2016

hmemcpy commented Jan 11, 2016

hmemcpy commented Jan 12, 2016

SzymonPobiega commented Jan 12, 2016

hmemcpy commented Jan 12, 2016

SzymonPobiega commented Jan 27, 2016

danielmarbach commented Jan 27, 2016

SzymonPobiega commented Jan 27, 2016

danielmarbach commented Jan 27, 2016

SzymonPobiega commented Jan 27, 2016

johnsimons commented Jan 27, 2016

SzymonPobiega commented Jan 27, 2016

SzymonPobiega commented Jan 29, 2016

danielmarbach Jan 29, 2016

andreasohlund commented Jan 29, 2016

danielmarbach commented Jan 29, 2016

danielmarbach commented Jan 29, 2016

mauroservienti commented Jan 29, 2016

danielmarbach Jan 29, 2016

danielmarbach commented Jan 29, 2016 via email

SzymonPobiega commented Feb 1, 2016

Scooletz commented Feb 1, 2016

SzymonPobiega commented Feb 1, 2016

SzymonPobiega commented Feb 2, 2016

danielmarbach commented Feb 2, 2016

SzymonPobiega commented Feb 2, 2016

SzymonPobiega commented Feb 2, 2016

SimonCropp commented Feb 10, 2016

danielmarbach commented Feb 10, 2016

SimonCropp commented Feb 10, 2016

danielmarbach commented Feb 10, 2016

SzymonPobiega commented Feb 10, 2016

SimonCropp commented Feb 10, 2016

andreasohlund commented Feb 10, 2016

Refactored serialization feature to be aligned with transport #3310

Refactored serialization feature to be aligned with transport #3310

Conversation

SzymonPobiega commented Jan 11, 2016

hmemcpy commented Jan 11, 2016

hmemcpy commented Jan 12, 2016

SzymonPobiega commented Jan 12, 2016

hmemcpy commented Jan 12, 2016

SzymonPobiega commented Jan 27, 2016

danielmarbach commented Jan 27, 2016

SzymonPobiega commented Jan 27, 2016

danielmarbach commented Jan 27, 2016

SzymonPobiega commented Jan 27, 2016

johnsimons commented Jan 27, 2016

SzymonPobiega commented Jan 27, 2016

SzymonPobiega commented Jan 29, 2016

danielmarbach Jan 29, 2016

Choose a reason for hiding this comment

andreasohlund commented Jan 29, 2016

danielmarbach commented Jan 29, 2016

danielmarbach commented Jan 29, 2016

mauroservienti commented Jan 29, 2016

danielmarbach Jan 29, 2016

Choose a reason for hiding this comment

danielmarbach commented Jan 29, 2016 via email

SzymonPobiega commented Feb 1, 2016

Scooletz commented Feb 1, 2016

SzymonPobiega commented Feb 1, 2016

SzymonPobiega commented Feb 2, 2016

danielmarbach commented Feb 2, 2016

SzymonPobiega commented Feb 2, 2016

SzymonPobiega commented Feb 2, 2016

SimonCropp commented Feb 10, 2016

danielmarbach commented Feb 10, 2016

SimonCropp commented Feb 10, 2016

danielmarbach commented Feb 10, 2016

SzymonPobiega commented Feb 10, 2016

SimonCropp commented Feb 10, 2016

andreasohlund commented Feb 10, 2016