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

ModelAndView that is not a Map #3113

Closed
satsen opened this issue Sep 2, 2023 · 8 comments · Fixed by #3395
Closed

ModelAndView that is not a Map #3113

satsen opened this issue Sep 2, 2023 · 8 comments · Fixed by #3395

Comments

@satsen
Copy link
Contributor

satsen commented Sep 2, 2023

I am using the jte.gg templating engine and it allows your model to be a class, so if I have a class called Model and want to pass this to ModelAndView I cannot do so because ModelAndView only takes a Map<String, Object>.

I tried to pass a Map.of("model", model) but the JTE file does not recognize this, the @param value becomes null. I am using precompiled templates with JTE. EDIT: That was due to another bug which I have submitted a fix for now, #3300.

@jknack
Copy link
Member

jknack commented Sep 4, 2023

found this too, but not sure how to fix it without introducing a break change. So probably this will be for 3.1 at least

Have you look here https://jooby.io/modules/jte/?

@param String name

<p>Hello ${name}!</p>
import io.jooby.jte.JteModule;

{
  install(new JteModule(Paths.of("src", "main", "jte")));         (1)

  get("/", ctx -> {
    return new ModelAndView("hello.jte", Map.of("name", "Jte"));  (2)
  });
}

@agentgt
Copy link
Contributor

agentgt commented Sep 4, 2023

Probably a better solution for JTE 3.0 and greater is to use https://github.com/casid/jte/tree/main/jte-models

And to do what Rocker and JStachio do of returning the model instance directly instead of ModelAndView.

@jknack
Copy link
Member

jknack commented Sep 4, 2023

need to look into models, yea. But we need something to mark it requires a template engine and not a message encoder... they work more or less the same in jooby, except we allow template engines to have more than one due they work by type (modelAndView) and by file extension

@agentgt
Copy link
Contributor

agentgt commented Sep 5, 2023

There is also the ResultHandler way as well. I was unaware of that way till I looked at Rocker's Jooby integration.

To be honest I'm not really sure when the ResultHandler way is picked over the MessageEncoder but from experimentation it appears that MVC picks MessageEncoder and programmatic/lambda picks ResultHandler. Is that right?

The ResultHandler seems to have a pro in that a buffer can be reused since it uses ByteBuffer instead of byte[] as well as more guarantees that the buffer is not shared I think. This is sort of related to #2100 .

@satsen
Copy link
Contributor Author

satsen commented Sep 8, 2023

Yeah @jknack it's pretty simple to do it in a backwards-compatible way. Just create a superclass for ModelAndView or interface that ModelAndView implements (name suggestions: CustomModelAndView or AnyModelAndView) and put the method Object getModel() there. You can decide if you want getModel() to have the return type Object or a type parameter T from the new superclass/interface, as Java allows subclasses to have return types that are subclasses of the parent method's return type.

Then of course you need to change the code and API to take this new superclass/interface as a param but that is backwards-compatible.

But if you want to do another method using a breaking change that is a possibility as well

@agentgt
Copy link
Contributor

agentgt commented Sep 8, 2023

@satsen FWIW that is more or less what we do albeit not for the exact reasons.
Our need is to hand over some attributes that our bound to the request to the model.

We have custom TemplateEngine and either extend the provided ones or make our own:

public interface ContextAwareModel {

    Map<String, @Nullable Object> getModel();

    default Map<String, @Nullable Object> getModel(
            Context ctx) {
        return addAttributes(ctx, getModel());
    }

    public static Map<String, @Nullable Object> addAttributes(
            Context ctx,
            Map<String, @Nullable Object> oldModel) {
        Map<String, @Nullable Object> model = new HashMap<>(ctx.getAttributes());
        model.putAll(oldModel);
        return model;
    }

}
//roughly... some things omitted
class CustomHandlebarsTemplateEngine implements TemplateEngine {

     @Override
    public String render(
            Context ctx,
            ModelAndView modelAndView)
            throws Exception {
        Template template = handlebars.compile(modelAndView.getView());
        Map<String, @Nullable Object> model;
        if (modelAndView instanceof ContextAwareModel cam) {
            model = cam.getModel(ctx);
        }
        else {
            model = ContextAwareModel.addAttributes(ctx, modelAndView.getModel());
        }
        var m = com.github.jknack.handlebars.Context.newBuilder(model)
            .resolver(
                    CustomMapValueResolver.CUSTOM_MAP_VALUE_RESOLVER,
                    RecordValueResolver.INSTANCE,
                    CustomJavaBeanValueResolver.INSTANCE,
                    EnumNameValueResolver.INSTANCE)
            .build();
        return template.apply(m);
    }

As you noted in your case though you will be returning something else other than a Map for getModel.

@jknack jknack added this to the 3.1.0 milestone Mar 17, 2024
jknack added a commit that referenced this issue Mar 18, 2024
- Allow to use anything as Model
- Remove `put()` methods from ModelAndView
- Introduce `MapModelAndView`
- Replace `new ModelAndView(String, Map)` by `ModelAndView.map()`
- fix #3113
@jknack
Copy link
Member

jknack commented Mar 18, 2024

Changes:

  • <T> getModel()
  • Remove all put method from ModelAndView
  • Replace ModelAndView(String) by ModelAndView.map(String)

See #3395

@satsen
Copy link
Contributor Author

satsen commented May 17, 2024

Thank you @jknack. Honestly I think it would have been better to make a getModel() superclass for ModelAndView and keep make ModelAndView extend that with OtherModel<Map<String, Object>> to keep backwards compatibilty but it is your choice and also too late now.

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

Successfully merging a pull request may close this issue.

3 participants