Support i18n in CC

[xingzhou]

This patch includes the code to introduce i18n into CC, contains:
(1). i18n init code
(2). Change the base controller to accept new HTTP_ACCEPT_LANGUAGE param
which identifies the locale of request client
(3). Enhanced ApiError class to translate the error message for REST API
response.

This patch does not include concrete REST API controller changes to
enable i18n. The changes of concrete REST API controllers' changes
will be submitted in other patches.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this. You can view the current status of your issue at: https://www.pivotaltracker.com/story/show/74023440.

[tedsuo]

@MarkKropf i18n in CC would continue the i18n support going into the CLI. I'm moving this to the Runtime tracker.

Thanks @xingzhou,

@tedsuo & @leoRoss
Community Pair

[xingzhou]

Thanks @tedsuo, there are still some follow on patches related to this patch. The following patches includes the translations in vendor/error project, the patches of each controller in CC to fully support i18n in CC. I will submit those patches these days, FYI.

[jpalermo]

@xingzhou Rather than making a new method in the controller, wouldn't it make more sense to modify the message method in the ApiError class? That way all errors will get translated correctly.

You will need to pull the HTTP_ACCEPT_LANGUAGE parameter out of the request and set it on I18N.locale, but that should be easy to do in the BaseController.

Dealing with specific error messages in the translate_validation_exception methods is still a bit tricky, but maybe the ApiError class can just take multiple translation keys. For example, in the AppsController, when there is an error with the number of instances being less than one, it could call:

Errors::ApiError.new_from_details("AppInvalid.instances_less_than_1")

and then the yml files could be structured like this:

AppInvalid:
  instances_less_than_1: "The app is invalid: Number of instances less than 1"

[xingzhou]

Hi @jpalermo , thanks for the comments!
Here is what I'm thinkging now, to discuss with you:

1. for the new method in ApiError, as there are a lot ot controllers in CC need to change in order to enable i18n, which I want to make in a group of PRs later(which are based on this PR merged), there should be fine to keep both the controllers i18n enabled and the controllers i18n not enabled both work well at the same time. As a result, I add a new method for those controllers i18n enabled to invoke. After all the controllers are i18n enabled in CC, we can just use the newly created method in API Error class and remove the old one.
2. for I18n.locale, I'm tending to think this is a global variable, as CC is serving different clients in parallel, it should use parameter among the methods to transfer the locale, if I'm wrong, please correct me.
3. For multi message key, I'm not clear what you mean, would you please help to give some more detail on why we need to do this? thanks

[jfmyers9]

I guess I'm not clear on what the translate_validation_exception_of_specific_language controller method is needed for. If you do all the error translation in ApiError directly. Individual controllers shouldn't need changes, they can continue to just raise ApiErrors, and the messages in those errors will get translated.

I18n.locale is a thread local, so each request can have a different value and they won't interfere with each other.

When I say message key, I'm referring to the string that gets passed to ApiError. For a key like AppInvalid, we normally pass along a second string to provide detail the error. Instead of passing a second string, we could expand it into a string with multiple translation keys in it. Such as AppInvalid.instances_less_than_1. And the I18n library with translate that if the structure of the yaml file is like this:

AppInvalid:
  instances_less_than_1: "The app is invalid: Number of instances less than 1"
  invalid_app_state: "The app is invalid: Invalid app state provided"
  unknown: "The app is invalid: %{errors_messages}"

For something like the unknown error case, you'd have to pass in the interpolation values in a hash:

Errors::ApiError.new_from_details("AppInvalid", error_messages: e.errors.full_messages)

[xingzhou]

at present, translate_validation_exception_of_specific_language is used to transfer the locale info, if we can use I18n.locale, we can directly use it in ApiError without adding such a new method.

I finally understood the error message stucture in CC, as there might be two segments in a single error message, I agree that we need to use the 'AppInvalid.instances_less_than_1' style to be the message id. By using this stucture of message id, seems we need to change the controllers code who is using Errors::ApiError.new_from_details method, for this changes, I'm suggesting we can do it later in other subsequent patches gradually.
I will try to make a new patch these days based on the above comments

[xingzhou]

Have submitted a change based on the above comments, please take a review, thx

[xingzhou]

Submitted some changes for review:

1. Use I18n.fallbacks to translate the message by default locale if the message is missing in the target locale or target locale is unknown to I18n
2. Keep using the original message from v2.yml in case the message is missing in the en_US locale. We can remove this exception catch after we have put all the messages used in CC into the vendor/errors/i18n/en_US.yml

[jpalermo]

This seems fine for now, but once this work is done this rescue should probably be removed so we don't accidentally have any error strings that are not in the translation files.

[xingzhou]

@jpalermo, yes, I agree with that, I'm thinking that the next patch for CC i18n will be extracting all the messages into vendor/error project and change the title of messages to sth like 'AppInvalid.instances_less_than_1'. Do I need to do something to let this PR merged now? thanks

[xingzhou]

Hello, team, anyone can help to push this PR forward? or anything I need to do to get this PR merged? thanks

[xingzhou]

@MarkKropf , We have discussed this PR last week, is there anybody from runtime team can take a look at this? I think we have finished the code review for this PR. Thanks!

[emalm]

@xingzhou, we've prioritized the corresponding story in the Runtime tracker, and we expect to be able to process it some time this week or early next week. Thanks again!

Best,
@ematpl, for the CF Runtime team

[xingzhou]

thanks @ematpl , anything need I do, please let me know