Various improvements #47
Conversation
| ServiceProvider serviceProvider = configurationSource.getPropertyOrDefault(SERVICE_PROVIDER_PROPERTY, ServiceProvider.class, val -> null, (serviceType) -> null); | ||
| this.templateEngine = serviceProvider.getService(ITemplateEngine.class).orElse(new TemplateEngine()); |
There was a problem hiding this comment.
Looks like NPEs are possible. Please throw some meaningful exception i.e. Required ServiceProvider is not available in configuration source. Alternatively, try to handle the null service provider maybe?
...arker/src/main/java/com/blazebit/notify/template/freemarker/FreemarkerTemplateProcessor.java
Show resolved
Hide resolved
| private F from; | ||
| private Long fromId; |
There was a problem hiding this comment.
I think you should remove the two fields and just leave a public abstract AbstractFromEmail getFrom() for a user to implement. AFAICS, you also don't need the type parameter. Users can covariantly override the contract.
| * The parameter name for the subject template processor key which is a plain {@link String}. | ||
| * The value refers to a template processor key. | ||
| */ | ||
| public static final String SUBJECT_TEMPLATE_PROCESSOR_KEY_PARAMETER = "subjectTemplateProcessorKey"; |
There was a problem hiding this comment.
I think this is a horrible pattern. People will usually only use a single template engine. So storing the template engine name for every email is just useless. The template engine should be resolved based on some resolver the user can override, which may receive access to the notification itself, so that the user can put the template engine name somehwere in case it isn't always the same.
We have to rethink this part a bit. We need something like this:
interface TemplateProcessorFactoryResolver {
TemplateProcessorFactory<T> resolveTemplateProcessorFactory(TemplateContext context, Class<T> resultType, Notification<?> notification, ConfigurationSource confiurationSource, ServiceProvider serviceProvider);
}There was a problem hiding this comment.
I think this interface is not needed as the user could just create a custom TemplateProcessorFactory that performs some kind of delegation. So maybe let's reduce the discussion to only allowing a single TemplateProcessorFactory per notification.
The issue is a different one: It must be possible to use different template processors for different parts of the same notification. Not sure how this interface would make this possible? If you only allow a single TemplateProcessorFactory then the caller must somehow provide information for what part of the notification the template processor is being created. I don't know how to do this in a generic fashion.
There was a problem hiding this comment.
The processor is based on the template name, no? And that one is configured per notification part. So if we resolve the TemplateProcessorFactory (representing the template engine) once and then call createTemplateProcessor for every notification part with the respective notification parts template name, then we should have proper TemplateProcessor objects to create the final parts.
There was a problem hiding this comment.
Well, we can do it that way but it basically shifts the responsibility to the user having to implement custom TemplateProcessorFactory where delegation is performed based on the templateName.
There was a problem hiding this comment.
IMO the user of blaze-notify should at most configure a base path where to find templates on the class path, but even for that I would suggest we use some sane default like e.g. templates/
| if (TemplateProcessor.TEMPLATE_NAME_PROPERTY.equals(key)) { | ||
| return templateName; | ||
| } else if (TemplateProcessor.SERVICE_PROVIDER_PROPERTY.equals(key)) { | ||
| return serviceProvider; | ||
| } |
There was a problem hiding this comment.
These two parameters should be part of the TemplateProcessorFactory#createTemplateProcessor method so that we don't have to construct a lambda every time.
There was a problem hiding this comment.
The EmailNotificationMessageResolver is cached in com.blazebit.notify.NotificationJobContext.Builder.DefaultNotificationJobContext#getNotificationMessageResolver(java.lang.Class<T>, com.blazebit.job.ConfigurationSource) so this code should not be executed every time.
Also, how do you suggest to pass these data items into the TemplateProcessorFactory?
There was a problem hiding this comment.
Just change the createTemplateProcessor method to accept these two additional parameters, then you don't need the lambda anymore.
| public class EmailNotification extends AbstractEmailNotification<Long> implements ConfigurationSourceProvider { | ||
|
|
||
| private static final long serialVersionUID = 1L; | ||
| public interface ServiceProvider { |
There was a problem hiding this comment.
Why do we need a new ServiceProvider interface and can't just use the existing one?
There was a problem hiding this comment.
The existing one is part of blaze-job-core-api. So I would have had to introduce a dependency to blaze-job in order to reuse this class which I thought doesn't make sense (why would the template module depend on the job module).
There was a problem hiding this comment.
The template stuff is just an integration aspect of blaze-notify which already depends on the job module, so it's fine to add another dependency here IMO
There was a problem hiding this comment.
Why did we duplicate com.blazebit.notify.template.api.ConfigurationSource then instead of just using com.blazebit.job.ConfigurationSource?
Mobe91
force-pushed
the
various_improvements
branch
2 times, most recently
from
2fa1190 to
cc204e5
Compare
Mobe91
force-pushed
the
various_improvements
branch
from
cc204e5 to
05d6548
Compare
No description provided.