[FP] ServiceContext fixes #6051
Conversation
josegar74
commented
Nov 22, 2021
josegar74
commented
- Keep service context independent to address user session null pointer exceptions (Keep service context independent to address user session null pointer exceptions #5507)
- Review and document use of ServiceContext (Review and document use of ServiceContext #5540)
josegar74
force-pushed
the
servicecontext-main
branch
from
2296032 to
970e353
Compare
- Keep service context independent to address user session null pointer exceptions (geonetwork#5507) - Review and document use of ServiceContext (geonetwork#5540)
josegar74
force-pushed
the
servicecontext-main
branch
from
970e353 to
4bee50d
Compare
| */ | ||
| public class ServiceContext extends BasicContext { | ||
| public class ServiceContext extends BasicContext implements AutoCloseable { |
There was a problem hiding this comment.
This looks good and is the key change, the above javadoc code example could include a try-with-resources example.
* ServiceContext is auto closable for use with try-with-resource syntaxt:
* <pre></code>
* try (ServiceContext context = serviceMan.createServiceContext("AppHandler", appContext)){
* ...
* }
* </code></pre>
There are other javadocs with this example, but it is nice to call it out in the class description.
| * is used during Jeeves startup and has some protection from being cleared. Additional managers such HarvestManager | ||
| * also use a shared service context to support background processes. | ||
| */ | ||
| public static class AppHandlerServiceContext extends ServiceContext { |
There was a problem hiding this comment.
suggestion only: Public static class could be broken out as its own top-level class, similar to BasicContext.
| */ | ||
| private static final ThreadLocalPolicy POLICY; | ||
| static { | ||
| String property = System.getProperty("jeeves.server.context.policy", "TRACE"); |
There was a problem hiding this comment.
I was only willing to default to TRACE for maintenance branch, on main we could consider defaulting to STRICT (at least for local development?).
| /** | ||
| * Language code, or <code>"?"</code> if undefined. | ||
| * @param lang language code, or <code>"?"</code> if undefined. | ||
| */ | ||
| public void setLanguage(final String lang) { | ||
| _language = lang; |
There was a problem hiding this comment.
| _language = lang; | |
| if( lang == null ){ | |
| lang = "?"; | |
| } | |
| _language = lang; |
| * IP address of request, or <code>"?"</code> for local loopback request. | ||
| * | ||
| * @param address ip, address or <code>"?"</code> for loopback request. | ||
| */ | ||
| public void setIpAddress(final String address) { | ||
| _ipAddress = address; |
There was a problem hiding this comment.
| _ipAddress = address; | |
| if( address == null ){ | |
| address = "?"; | |
| } | |
| _ipAddress = address; |
| // session.loginAs(new User().setUsername("admin").setId(-1).setProfile(Profile.Administrator)); | ||
| // LOGGER_DATA_MANAGER.debug("Hopefully this is cron job or routinely background task. Who called us?", | ||
| // new Exception("Dummy Exception to know the stacktrace")); | ||
| // } |
There was a problem hiding this comment.
This could should just be removed, or if we like on main a failure could be raised.
if (context.getUserSession() == null) {
throw new IllegalStateException( "Anonymous data manager access not available");
}
There was a problem hiding this comment.
I was being so careful not to break existing workflow I did not introduce any additional strict checking.
| try (ServiceContext context = ApiUtils.createServiceContext(httpRequest)) { | ||
| Locale locale = languageUtils.parseAcceptLanguage(request.getLocales()); | ||
| String language = isoLanguagesMapper.iso639_2T_to_iso639_2B(locale.getISO3Language()); | ||
| return translationPackBuilder.getPack(language, pack, context); | ||
| } |
There was a problem hiding this comment.
| try (ServiceContext context = ApiUtils.createServiceContext(httpRequest)) { | |
| Locale locale = languageUtils.parseAcceptLanguage(request.getLocales()); | |
| String language = isoLanguagesMapper.iso639_2T_to_iso639_2B(locale.getISO3Language()); | |
| return translationPackBuilder.getPack(language, pack, context); | |
| } | |
| try (ServiceContext context = ApiUtils.createServiceContext(httpRequest)) { | |
| Locale locale = languageUtils.parseAcceptLanguage(request.getLocales()); | |
| String language = isoLanguagesMapper.iso639_2T_to_iso639_2B(locale.getISO3Language()); | |
| return translationPackBuilder.getPack(language, pack, context); | |
| } |
There was a problem hiding this comment.
Outdenting the try / finally statement by two characters can vastly reduce the number of lines in the diff as the origional logic remains unchanged.
|
@josegar74 I resolved the outstanding merge conflicts, please check. |
|
|
|
@josegar74 while I provided feedback above this PR remains open. May it be closed? |
