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

Specialised Generic decorator doesn't work for pure open generic registration #27

Closed
LordZoltan opened this Issue Apr 2, 2017 · 1 comment

Comments

Projects
None yet
1 participant
@LordZoltan
Collaborator

LordZoltan commented Apr 2, 2017

Disappointingly, I missed a test in the DecoratorTargetTests and, now, whilst in the process of writing the examples for #12 (see DecoratorExamples.ShouldUseSpecialisedDecoratorForMyService2 - I tried the equivalent of this (pseudo code):

RegisterType(Foo<>, IFoo<>)
RegisterDecorator(FooStringDecorator, IFoo<string>)

Resolve(IFoo<string>) //expected: FooStringDecorator with Foo<string> inner

The result is not decorated with the FooStringDecorator as expected.

The reason this does not work, but the various generic decorators in DecoratorTargetTests do, is because those tests don't use open generics for the underlying Type registrations - only for the decorators.

But it turns out those tests are wrong because the order the decorators are applied in is wrong - and I cannot believe I missed it! In all cases, the specialised generic decorator should be applied last but is, in fact, applied first.

As it stands, then, there's no point in adding the open generic example to #12 and moving it to 'Done' because the only way to get it working is if you create individual registrations for the type you know you're going to be decorating - which is pointless.

Fix

Not precisely sure of the fix at the moment, however, I think the clue is in the way the already established tests demonstrate the decorator order getting screwed up.

Instinct tells me I just need to make sure that a DecoratorTargetContainer always gets merged with the topmost target in the TargetContainer its registered into; and it'll fix it.

Next steps

  • The page being built for #12 will not include the open generic test, but include a reference to this bug, explaining that it will be fixed in 1.2
  • The test itself will be left in the examples project so that we have a failing test to work from which we can then immediately use. Another will also be added to the DecoratorTargetTests in the Rezolver.Tests.Specification project
  • The existing decorators tests will be altered so that they fail as they should.
    • They should actually be rewritten to use type checking like the ones I've written for #12

Once fixed

  • Make sure #12 is updated with the example!

@LordZoltan LordZoltan added this to the 1.2 milestone Apr 2, 2017

@LordZoltan LordZoltan self-assigned this Apr 2, 2017

LordZoltan added a commit that referenced this issue Apr 2, 2017

#12 almost done, but completion will be delayed till 1.2 due to #27 b…
…eing discovered whilst writing the documentation. Two tests in DecoratorTargetTests have been 'fixed' (and now fail) plus the test intended as example 5 for decorators also doesn't pass. All three tests are there as ready-made test cases for 27

@LordZoltan LordZoltan added in progress and removed ready labels Apr 19, 2017

LordZoltan added a commit that referenced this issue Apr 19, 2017

tidying up the decorator compiler tests for #27
moving to theories and switching to pure type testing instead of magic string checking (too error prone).  Also rationalised all the test type names.
@LordZoltan

This comment has been minimized.

Collaborator

LordZoltan commented Apr 20, 2017

Implementation notes

DecoratingTargetContainer was changed to apply a simple type matching rule to determine if it should apply the decorator to the target returned by a Fetch call (diff)

RegisterDecorator was changed so that if the type is a closed generic, it still treats the decorator as being registered as the open generic type. This means that every single decorator effectively pushes a deeper stack of decorating containers - a new level for each new decorator that's registered (diff)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment