ResourceHandler does not provide defaultLogger

[flaxel]

It's possible that extending ResourceHandler is improperly generated because that is only supported by "configuration resources" (declared in vlingo-http.properties). For dynamic resources handlers the Stage must be passed in via constructor. With the Stage the dynamic resources handlers can get to anything needed, including the Logger.

/cc @danilo-ambrosio @Florian-Schoenherr

[Florian-Schoenherr]

Could you use this for now?

private final Stage stage;
private final Logger logger;
public SomeResource(final Stage stage) {
       this.stage = stage;
       this.logger = stage.world().defaultLogger();
}

If you want to generate more Resources right now, I could change the generation to include these lines. If you just need it on one or two, or on already generated Resources, you can copy it in. Sorry, I really thought the logger() from ResourceHandler was usable like that! I will keep this open until this is resolved in a better way. This will suffice for now.

[flaxel]

Oh yeah, that's okay. 👍 Thank you for your fast support! 😄

[VaughnVernon]

@Florian-Schoenherr Has the code generation been fixed such that it supports your suggestion?

https://github.com/vlingo/vlingo-xoom/issues/4#issuecomment-714629090

[Florian-Schoenherr]

@VaughnVernon I kept this open mainly because #5 was not the most optimal solution. We could close, but I think there is still some misconception of extends ResourceHandler: In http docs: In xoom docs: And as said in #5, ServerActor doesn't know about routes without extends ResourceHandler, which means ResourceHandler is either used wrong or should have a stage (&context).

The fix I applied is just "here, have another logger".

[VaughnVernon]

Thanks, @Florian-Schoenherr

@danilo-ambrosio When you have an chance, please address this. I don't know where the problem is:

• Does the @ResourceHandler annotation require extends ResourceHandler? If so, that is incorrect. When routes() is used the resource handler class is freed from the use of extends ResourceHandler because it is now a DynamicResourceHandler rather than a ConfigurationResourceHandler (created by using the definition in the vlingo-http.properties).

• Is the Starter code generation generating the extends ResourceHandler? I think in either case, using annotations or not, this would be incorrect. My understanding is both when using annotations and not, the resource handler is always configured via routes() and thus extend ResourceHandler would never be appropriate.

The problem is that when extends ResourceHandler is used with a DynamicResourceHandler it will never be initialized for each request that is handled, and thus, trying to access the inherited members will cause NullPointerExceptions.

[Florian-Schoenherr]

I tried removing extends ResourceHandler in schemata, e.g. right here, which doesn't work. That's the only thing I really know. (also, routes does Override ResourceHandler::routes())

[VaughnVernon]

If you remove extends ResourceHandler then routes() is no longer an @Override. In other words, just remove the @Override and you should be good to go. I don't understand why this would be a problem otherwise.

[Florian-Schoenherr]

That did not work. Maybe it was something specific to schemata, my guess is that xoom's @ResourceHandlers(packages = "io.vlingo.schemata.resource") looks at everything inside io.vlingo.schemata.resource that extends ResourceHandler. @danilo-ambrosio is that correct?

[danilo-ambrosio]

You're right, @Florian-Schoenherr.

ResourceHandler is also used as a key, speeding up the search for valid Resource classes inside a given package. I wasn't aware of the impacts @VaughnVernon mentioned when you extend ResourceHandler, but I have a straightforward solution. I can simply create a markup abstract class on vlingo-xoom for that purpose. Thus, we can maintain the package attribute of @ResourceHandlers annotation, which ease the resource handlers mapping.

What do you think? @VaughnVernon @Florian-Schoenherr

[VaughnVernon]

@danilo-ambrosio Do you mean a marker class/interface? I prefer not to use one. However, you could put a meaningful method on the interface:

public interface DynamicResourceHandler {
  Resource<?> routes();
}

Now overriding Resource<?> routes() in the implementing class makes sense.

[Florian-Schoenherr]

Well, I know why marker-classes are not so nice. Also, aren't annotations supposed to be used for marking (too)? Then again, having annotations on every resource class is also not what we are going for.

The interface with routes() from @VaughnVernon makes sense, maybe we could make it an abstract class and provide the logger(), too. Although having thought about it, generating the logger like this.$logger = $stage.world().defaultLogger(); makes more sense, because beginners will then have it right there and you can still just delete it if you didn't need it. So we just want routes().

@danilo-ambrosio do we need anything else on the marker class/interface?

[VaughnVernon]

@Florian-Schoenherr @danilo-ambrosio It could be an abstract base that has a constructor that requires the Stage and holds that and the logger. That's fine. I don't think it matters much in terms of code gen.

public abstract class DynamicResourceHandler {
  private final Logger logger;
  private final Stage stage;

  public abstract Resource<?> routes();

  protected DynamicResourceHandler(final Stage stage) {
    this.stage = stage;
    this.logger = stage.world().defaultLogger();
  }

  protected Logger logger() {
    return logger;
  }

  protected Stage stage() {
    return stage;
  }
}

[danilo-ambrosio]

@Florian-Schoenherr

We could use annotations but we still need to ensure some methods implementation on resources classes.

If we use annotations instead of marker interface/class, then a compile-time validation is required for making sure the right methods are implemented. That takes time to be implemented.

So I vote for @VaughnVernon 's suggestion which seems to be simpler. Note that Stage needs to be "injected" because, when using xoom annotation, developers are not able to do it by editing the Bootstrap class, which is automatically generated.

[VaughnVernon]

@danilo-ambrosio Do you mean injected as in passing a constructor parameter from the generated Bootstrap or by another means? Seems like all generated DynamicResourceHandler extenders should have a single constructor generated that takes a Stage and gives it to DynamicResourceHandler via super(stage).

[danilo-ambrosio]

Yes, "injected" means exactly what you've just said, @VaughnVernon . Thanks for clarifying.

[Florian-Schoenherr]

need to ensure some methods implementation

then obviously no annotation 👍 And yes, super(stage)!

[danilo-ambrosio]

@Florian-Schoenherr @VaughnVernon

If you get a chance, please review my changes below and let me know if everything is right:

• VLINGO/HTTP: https://github.com/vlingo/vlingo-http/commit/6d39b9df050483fe237563b6b903b44fe8d52498
• VLINGO/XOOM: https://github.com/vlingo/vlingo-xoom/commit/3fa9f63a6aaee4056065ff047721ecc96176de86
• VLINGO/STARTER: https://github.com/vlingo/vlingo-xoom-starter/commit/020272e93e7faf23f29868b401696b490e0097c2

After the review, could @Florian-Schoenherr please update VLINGO/SCHEMATA REST resources by extending DynamicResourceHandler ?

[Florian-Schoenherr]

Works as expected! 👍

[VaughnVernon]

@danilo-ambrosio @Florian-Schoenherr I am closing this. Note that I have added support for accessing Context from DynamicResourceHandler. To activate it, code must be generated to include a self reference in routes() when instantiating the ResoureBuilder.