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

[FIX JENKINS-35845] Internationalisation for Blue Ocean and JDL #2586

Merged
merged 12 commits into from
Oct 22, 2016
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions core/src/main/java/hudson/util/HttpResponses.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import javax.servlet.ServletException;
import java.io.File;
Expand Down
47 changes: 46 additions & 1 deletion core/src/main/java/jenkins/util/ResourceBundleUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,16 @@
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import hudson.PluginWrapper;
import java.util.logging.Logger;
import java.util.Locale;
import java.util.Map;
import java.util.MissingResourceException;
import java.util.ResourceBundle;
import java.util.concurrent.ConcurrentHashMap;
import jenkins.model.Jenkins;

/**
* Simple {@link java.util.ResourceBundle} utility class.
Expand All @@ -42,6 +46,7 @@
@Restricted(NoExternalUse.class)
public class ResourceBundleUtil {

private static final Logger logger = Logger.getLogger("jenkins.util.ResourceBundle");
private static final Map<String, JSONObject> bundles = new ConcurrentHashMap<>();

private ResourceBundleUtil() {
Expand Down Expand Up @@ -72,14 +77,54 @@ private ResourceBundleUtil() {
return bundleJSON;
}

ResourceBundle bundle = ResourceBundle.getBundle(baseName, locale);
ResourceBundle bundle = getBundle(baseName, locale, Jenkins.class.getClassLoader());
if (bundle == null) {
// Not in Jenkins core. Check the plugins.
Jenkins jenkins = Jenkins.getInstance(); // will never return null
if (jenkins != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is not required

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it is @oleg-nenashev the failure you have linked is provoked by that test not being in place anymore.

for (PluginWrapper plugin : jenkins.getPluginManager().getPlugins()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally needs to be optimized. IIRC baseName usually contains /plugin/myPlugin, which may tweak the performance

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure how to optimize though, some hints?

bundle = getBundle(baseName, locale, plugin.classLoader);
if (bundle != null) {
break;
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder should we be specifying the actual plugin name as a parameter on the rest endpoint?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this is ok ... keeps the code more simple and is only executed once (if the bundle is not in the cache).

if (bundle == null) {
throw new MissingResourceException("Can't find bundle for base name "
+ baseName + ", locale " + locale, baseName + "_" + locale, "");
}

bundleJSON = toJSONObject(bundle);
bundles.put(bundleKey, bundleJSON);

return bundleJSON;
}

/**
* Get a plugin bundle using the supplied Locale and classLoader
*
* @param baseName The bundle base name.
* @param locale The Locale.
* @param classLoader The classLoader
* @return The bundle JSON.
*/
private static @CheckForNull ResourceBundle getBundle(@Nonnull String baseName, @Nonnull Locale locale, @Nonnull ClassLoader classLoader) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, @jglick-style violates the recommended code style

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oleg-nenashev not sure what you mean

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well...

Style defined in https://wiki.jenkins-ci.org/display/JENKINS/Beginners+Guide+to+Contributing#BeginnersGuidetoContributing-CodeStyle

@CheckForNull 
private static ResourceBundle getBundle(@Nonnull String baseName, @Nonnull Locale locale, @Nonnull ClassLoader classLoader)

jglick-style:

private static @CheckForNull ResourceBundle getBundle(@Nonnull String baseName, @Nonnull Locale locale, @Nonnull ClassLoader classLoader)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oleg-nenashev ok got ya, so should I fix the other occurrence of that in the file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the file contains only one annotation style.

Then I'd just keep it as is and then perform a bulk refactoring when Jesse goes to a vacation without internet access :)

try {
return ResourceBundle.getBundle(baseName, locale, classLoader);
} catch (MissingResourceException e) {
// fall through and return null.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐜 Must go to system log

logger.warning(e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really deserves warning? IIUC it is ok if particular plugin just does not provide it.

In case no plugin provide it we will have one exception thrown and N warning records logged. If it is provided in the last plugin, there will be N-1 warnings for no reason.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we just need to reduce the message severity to the FINE or FINEST level

Copy link

@i386 i386 Nov 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oleg-nenashev is this the only update that is blocking this from backporting?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Blocking" - maybe. But we still need to agree regarding the feasibility of the backporting

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, will this print an average of dozens of messages every time a non-core resource is searched? It iterates over all plugins… If so, this should be changed in master as well.

Copy link
Member

@oleg-nenashev oleg-nenashev Nov 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should. Creating the issue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
return null;
}

/**
* Create a JSON representation of a resource bundle
*
* @param bundle The resource bundle.
* @return The bundle JSON.
*/
private static JSONObject toJSONObject(@Nonnull ResourceBundle bundle) {
JSONObject json = new JSONObject();
for (String key : bundle.keySet()) {
Expand Down
19 changes: 16 additions & 3 deletions test/src/test/java/jenkins/I18nTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
*/
package jenkins;

import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
import net.sf.json.JSONObject;
import org.junit.Assert;
import org.junit.Rule;
Expand All @@ -49,9 +50,21 @@ public void test_baseName_unspecified() throws IOException, SAXException {

@Test
public void test_baseName_unknown() throws IOException, SAXException {
JSONObject response = jenkinsRule.getJSON("i18n/resourceBundle?baseName=com.acme.XyzWhatever").getJSONObject();
Assert.assertEquals("error", response.getString("status"));
Assert.assertTrue(response.getString("message").contains("com.acme.XyzWhatever"));
try {
JSONObject response = jenkinsRule.getJSON("i18n/resourceBundle?baseName=com.acme.XyzWhatever").getJSONObject();
} catch (FailingHttpStatusCodeException e) {
Assert.assertNotNull(e);
Assert.assertTrue(e.getMessage().contains("com.acme.XyzWhatever"));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm .... maybe check for something more specific?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just looked and I think you should get a HtmlUnit exception (FailingHttpStatusCodeException I think) that you can check i.e. check for an expected exception Vs just any exception. If it is FailingHttpStatusCodeException, you should still be able to get the actual response and perform the same checks that were removed, I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not feel this test is enough for the change

}

@Test
public void test_baseName_plugin() throws IOException, SAXException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice2have: Add @Issue() reference

// ssh-slaves plugin is installed by default
JSONObject response = jenkinsRule.getJSON("i18n/resourceBundle?baseName=hudson.plugins.sshslaves.Messages").getJSONObject();
Assert.assertEquals("ok", response.getString("status"));
JSONObject data = response.getJSONObject("data");
Assert.assertEquals("The launch timeout must be a number.", data.getString("SSHConnector.LaunchTimeoutMustBeANumber"));
}

@Test
Expand Down