Encode logfile option with URLEncoder #1363
Conversation
|
Hi @jasonxh, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement: |
|
Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK ΤO ΤESΤ' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged. For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask! |
|
OK TO TEST |
|
@hithran @orendain @eshepelyuk thoughts? |
|
| @@ -45,7 +46,8 @@ object Main extends App { | |||
| private val extraGenerators: List[String] = | |||
| parameters.getOrElse("extra_generators", "").split(";").toList.filter(_ != "") | |||
|
|
|||
| private val logger = parameters.get("logfile").map(new FileLogger(_)).getOrElse(SilencedLogger) | |||
| private val logger: Logger = | |||
| parameters.get("logfile_enc").map(URLDecoder.decode(_, "utf-8")).map(new FileLogger(_)).getOrElse(SilencedLogger) | |||
There was a problem hiding this comment.
can we fall back to the logfile parameter when logfile_enc is not present?
There was a problem hiding this comment.
Added fallback logic
@jasonxh Great solution! Thanks! |
References #786
Issue
Currently, applying the gradle plugin to a subproject will fail gradle build from root project dir, due to a mismatch in the expectation of where the
logfileis located. The gradle plugin calculates a relative path to the subproject, while the protoc codegen plugin evaluates it as relative to the current working dir (which is root dir).Background
There had been several back-and-forths regarding how to pass the
logfileoption in the past, ranging from one-off character substitution, to relative paths, to absolute paths (see #674 and #787). The root of the problem is that protoc codegen plugin options are passed in as a single string, encoded in a special format like this:It's easy to see how several characters play a special role in this encoding, and thus should not appear in keys or values:
[=,:].This problem was originally surfaced as a Windows-only problem, where absolute file paths include
C:\(see #669). However, it doesn't actually matter if we are passinglogfileas absolute or relative, since those special characters mentioned above can creep in either way. Granted, no sane person would name their files like that, but it's possible.Proposed fix
Encode
logfilewith URLEncoder when composing options, and decode it with URLDecoder inside the protoc codegen plugin. This is a well-known encoding scheme that allows only[a-zA-Z0-9.-*_+%]in the encoded output. Note that none of the special characters mentioned above are allowed here.I also renamed the option to
logfile_encto make it an explicit breaking change, in case the codegen plugin is being used in other context. But I don't feel strongly about this, so feel free to suggest otherwise.Testing
Verified to fix #786 on my Mac. Would be great if someone can help test this on a Windows machine.