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 5 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
86 changes: 65 additions & 21 deletions core/src/main/java/hudson/util/HttpResponses.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,28 +23,30 @@
*/
package hudson.util;

import java.io.File;
Copy link
Member

Choose a reason for hiding this comment

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

Please never use IDE's autoformat :(

import java.io.IOException;
import java.nio.charset.Charset;
import java.util.Map;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import javax.servlet.ServletException;
import net.sf.json.JSONArray;
import net.sf.json.JSONObject;
import org.kohsuke.stapler.HttpResponse;
import org.kohsuke.stapler.StaplerRequest;
import org.kohsuke.stapler.StaplerResponse;

import javax.annotation.Nonnull;
import javax.servlet.ServletException;
import java.io.File;
import java.io.IOException;
import java.nio.charset.Charset;
import java.util.Map;

/**
* Various {@link HttpResponse} implementations.
*
* <p>
* This class extends from Stapler so that we can move implementations from here to Stapler periodically.
* This class extends from Stapler so that we can move implementations from here
* to Stapler periodically.
*
* @author Kohsuke Kawaguchi
*/
public class HttpResponses extends org.kohsuke.stapler.HttpResponses {

public static HttpResponse staticResource(File f) throws IOException {
return staticResource(f.toURI().toURL());
}
Expand All @@ -60,6 +62,7 @@ public static HttpResponse okJSON() {

/**
* Create a response containing the supplied "data".
*
* @param data The data.
*
* @since 2.0
Expand All @@ -70,6 +73,7 @@ public static HttpResponse okJSON(@Nonnull JSONObject data) {

/**
* Create a response containing the supplied "data".
*
* @param data The data.
*
* @since 2.0
Expand All @@ -80,35 +84,53 @@ public static HttpResponse okJSON(@Nonnull JSONArray data) {

/**
* Create a response containing the supplied "data".
*
* @param data The data.
*
* @since 2.0
*/
public static HttpResponse okJSON(@Nonnull Map<?,?> data) {
public static HttpResponse okJSON(@Nonnull Map<?, ?> data) {
return new JSONObjectResponse(data);
}

/**
* Set the response as an error response.
* @param message The error "message" set on the response.
* @return {@link this} object.
*
* @since 2.0
*/
/**
* Set the response as an error response.
*
* @param message The error "message" set on the response.
* @return {@link this} object.
*
* @since 2.0
*/
public static HttpResponse errorJSON(@Nonnull String message) {
return new JSONObjectResponse().error(message);
}

/**
* Set the response as an error response.
*
* @param message The error "message" set on the response.
* @param errorCode The HTTP error code to return;
* @return {@link this} object.
*
* @since TODO
*/
public static HttpResponse errorJSON(@Nonnull String message, int errorCode) {
Copy link
Member

@oleg-nenashev oleg-nenashev Oct 12, 2016

Choose a reason for hiding this comment

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

Maybe YAGNI, but OK

return new JSONObjectResponse().error(message).withStatusCode(errorCode);
}

/**
* {@link net.sf.json.JSONObject} response.
*
* @author <a href="mailto:tom.fennelly@gmail.com">tom.fennelly@gmail.com</a>
* @author
* <a href="mailto:tom.fennelly@gmail.com">tom.fennelly@gmail.com</a>
*/
static class JSONObjectResponse implements HttpResponse {

private static final Charset UTF8 = Charset.forName("UTF-8");

private final JSONObject jsonObject;
@CheckForNull
private Integer statusCode;
Copy link
Member

Choose a reason for hiding this comment

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

🐜 Add @CheckForNull.


/**
* Create an empty "ok" response.
Expand All @@ -120,6 +142,7 @@ static class JSONObjectResponse implements HttpResponse {

/**
* Create a response containing the supplied "data".
*
* @param data The data.
*/
JSONObjectResponse(@Nonnull JSONObject data) {
Expand All @@ -129,6 +152,7 @@ static class JSONObjectResponse implements HttpResponse {

/**
* Create a response containing the supplied "data".
*
* @param data The data.
*/
JSONObjectResponse(@Nonnull JSONArray data) {
Expand All @@ -138,33 +162,50 @@ static class JSONObjectResponse implements HttpResponse {

/**
* Create a response containing the supplied "data".
*
* @param data The data.
*/
JSONObjectResponse(@Nonnull Map<?,?> data) {
JSONObjectResponse(@Nonnull Map<?, ?> data) {
this();
this.jsonObject.put("data", JSONObject.fromObject(data));
}

/**
* Set the response status code.
*
* @param statusCode The status code.
* @return {@link this} object.
*/
public JSONObjectResponse withStatusCode(Integer statusCode) {
Copy link
Member

Choose a reason for hiding this comment

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

Also @CheckForNull for parameter? NIT

this.statusCode = statusCode;
return this;
}

/**
* Set the response as an error response.
*
* @param message The error "message" set on the response.
* @return {@link this} object.
*/
@Nonnull JSONObjectResponse error(@Nonnull String message) {
@Nonnull
JSONObjectResponse error(@Nonnull String message) {
status("error");
this.jsonObject.put("message", message);
return this;
}

/**
* Get the JSON response object.
*
* @return The JSON response object.
*/
@Nonnull JSONObject getJsonObject() {
@Nonnull
JSONObject getJsonObject() {
return jsonObject;
}

private @Nonnull JSONObjectResponse status(@Nonnull String status) {
private @Nonnull
JSONObjectResponse status(@Nonnull String status) {
this.jsonObject.put("status", status);
return this;
}
Expand All @@ -175,6 +216,9 @@ static class JSONObjectResponse implements HttpResponse {
@Override
public void generateResponse(StaplerRequest req, StaplerResponse rsp, Object node) throws IOException, ServletException {
byte[] bytes = jsonObject.toString().getBytes(UTF8);
if (statusCode != null) {
rsp.setStatus(statusCode);
}
rsp.setContentType("application/json; charset=UTF-8");
rsp.setContentLength(bytes.length);
rsp.getOutputStream().write(bytes);
Expand Down
6 changes: 5 additions & 1 deletion core/src/main/java/jenkins/I18n.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@
import org.kohsuke.stapler.HttpResponse;
import org.kohsuke.stapler.StaplerRequest;

import javax.servlet.http.HttpServletResponse;
import java.util.Locale;
import java.util.MissingResourceException;

/**
* Internationalization REST (ish) API.
Expand Down Expand Up @@ -106,8 +108,10 @@ public HttpResponse doResourceBundle(StaplerRequest request) {
}

return HttpResponses.okJSON(ResourceBundleUtil.getBundle(baseName, locale));
} catch (MissingResourceException e) {
return HttpResponses.errorJSON(e.getMessage(), HttpServletResponse.SC_NOT_FOUND);
} catch (Exception e) {
return HttpResponses.errorJSON(e.getMessage());
return HttpResponses.errorJSON(e.getMessage(), HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
Copy link
Member

Choose a reason for hiding this comment

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

I see what you are doing here and it makes sense, but strictly speaking is not needed for fixing the issue wrt loading bundles from plugins, or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tfennelly you are right strictly this is not needed.

Copy link
Member

Choose a reason for hiding this comment

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

Better to submit as a separate PR then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will refactor to drop the part on SC_INTERNAL_SERVER_ERROR

}
}
}
62 changes: 54 additions & 8 deletions core/src/main/java/jenkins/util/ResourceBundleUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,22 @@
*/
package jenkins.util;

import net.sf.json.JSONObject;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import javax.annotation.Nonnull;
import hudson.PluginWrapper;
import java.util.Locale;
import java.util.Map;
import java.util.MissingResourceException;
import java.util.ResourceBundle;
import java.util.concurrent.ConcurrentHashMap;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import jenkins.model.Jenkins;
import net.sf.json.JSONObject;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

/**
* Simple {@link java.util.ResourceBundle} utility class.
*
* @author <a href="mailto:tom.fennelly@gmail.com">tom.fennelly@gmail.com</a>
* @since 2.0
*/
Expand All @@ -49,37 +52,80 @@ private ResourceBundleUtil() {

/**
* Get a bundle JSON using the default Locale.
*
* @param baseName The bundle base name.
* @return The bundle JSON.
* @throws MissingResourceException Missing resource bundle.
*/
public static @Nonnull JSONObject getBundle(@Nonnull String baseName) throws MissingResourceException {
public static @Nonnull
JSONObject getBundle(@Nonnull String baseName) throws MissingResourceException {
return getBundle(baseName, Locale.getDefault());
}

/**
* Get a bundle JSON using the supplied Locale.
*
* @param baseName The bundle base name.
* @param locale The Locale.
* @return The bundle JSON.
* @throws MissingResourceException Missing resource bundle.
*/
public static @Nonnull JSONObject getBundle(@Nonnull String baseName, @Nonnull Locale locale) throws MissingResourceException {
public static @Nonnull
JSONObject getBundle(@Nonnull String baseName, @Nonnull Locale locale) throws MissingResourceException {
String bundleKey = baseName + ":" + locale.toString();
JSONObject bundleJSON = bundles.get(bundleKey);

if (bundleJSON != null) {
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
for (PluginWrapper plugin : jenkins.getPluginManager().getPlugins()) {
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.
* @throws MissingResourceException Missing resource bundle.
Copy link
Member

Choose a reason for hiding this comment

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

🐛 It's not being thrown from the method

*/
private static @Nullable
Copy link
Member

Choose a reason for hiding this comment

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

Strange formatting

Copy link
Member

Choose a reason for hiding this comment

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

Should be CheckForNull since it may happen on a common path

ResourceBundle getBundle(@Nonnull String baseName, @Nonnull Locale locale, @Nonnull ClassLoader classLoader) {
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

}
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
20 changes: 15 additions & 5 deletions test/src/test/java/jenkins/I18nTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,14 @@
*/
package jenkins;

import java.io.IOException;
import net.sf.json.JSONObject;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.JenkinsRule;
import org.xml.sax.SAXException;

import java.io.IOException;

/**
* @author <a href="mailto:tom.fennelly@gmail.com">tom.fennelly@gmail.com</a>
*/
Expand All @@ -49,9 +48,20 @@ 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 (Exception e) {
Assert.assertNotNull(e);
}
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