New hooks (issue 106) #113
Conversation
… that, while inert, isn't necessary for this change
… params can be used
| Change change = new Change(id, null, description, f.getAbsolutePath()); | ||
| Properties props = environmentProperties(); | ||
|
|
||
| hookBindings.put("change", change); |
There was a problem hiding this comment.
new hooks pass the change, environment, paths and extra CLI args
| @@ -13,7 +13,7 @@ | |||
| * See the License for the specific language governing permissions and | |||
| * limitations under the License. | |||
| */ | |||
| package org.apache.ibatis.migration.hook; | |||
| package org.apache.ibatis.migration.hook.scripts; | |||
There was a problem hiding this comment.
I can move these back to their original package if desired but there seems to be enough related code to warrant a new package
| @@ -108,7 +108,7 @@ public void execute(Map<String, Object> bindingMap) { | |||
| if (functionName != null) { | |||
| printStream.println(Util.horizontalLine("Invoking function : " + functionName, 80)); | |||
| invocable.invokeFunction(functionName, args.toArray()); | |||
| } else if (objectName != null && methodName != null) { | |||
There was a problem hiding this comment.
This was entirely superflous
|
I have implemented the |
|
Hi @chb0github , I'm sorry for a late reply. |
|
Hmm, How can I minimize it? What I can do is line-by-line explain it. That should help. I have given a line-by-line explanation. Hope that helps |
| @@ -37,6 +37,13 @@ public Change(BigDecimal id, String appliedTimestamp, String description) { | |||
| this.description = description; | |||
| } | |||
|
|
|||
| public Change(BigDecimal id, String appliedTimestamp, String description, String filename) { | |||
There was a problem hiding this comment.
Added this so I could have a single constructor that takes all params and not require a setter to add the the filename (I think that was the field missing)
| @@ -71,7 +78,8 @@ public void setFilename(String filename) { | |||
|
|
|||
| @Override | |||
| public String toString() { | |||
| return id + " " + (appliedTimestamp == null ? " ...pending... " : appliedTimestamp) + " " + description; | |||
| String ts = appliedTimestamp == null ? " ...pending... " : appliedTimestamp; | |||
There was a problem hiding this comment.
It was just easier for me to read
| @@ -52,7 +52,9 @@ | |||
| hook_before_down, | |||
| hook_before_each_down, | |||
| hook_after_each_down, | |||
| hook_after_down | |||
| hook_after_down, | |||
There was a problem hiding this comment.
Follows the current design for hooks. Nothing new here just adding code to support "new"
| @@ -113,7 +117,9 @@ public Environment(File file) { | |||
| this.username = prop.getProperty(SETTING_KEY.username.name()); | |||
| this.password = prop.getProperty(SETTING_KEY.password.name()); | |||
|
|
|||
| this.hookBeforeUp = prop.getProperty(SETTING_KEY.hook_before_up.name()); | |||
| this.hookBeforeNew = prop.getProperty(SETTING_KEY.hook_before_new.name()); | |||
There was a problem hiding this comment.
Follows current design. Importantly, there is no such thing as a before_each or after_each in the case of a new. So, I don't have analogs here for up/down
| * Use this to define template variables | ||
| * @return combined properties of system/environment with 'sys.' and 'env.' appended respectively | ||
| */ | ||
| protected Properties getVariables() { |
There was a problem hiding this comment.
The templating system for new only had description available which was really limited. In the case I have in mine I need some environment variables. This falls in line with typical template substitutions. So, in your new script you can do this: insert into ${env.USER} this was previously not available. Because I put it in BaseCommand it is available to all commands now to integrate.
| @@ -152,7 +174,7 @@ private String generatePatternedId(String pattern) { | |||
| } | |||
| } | |||
|
|
|||
| private String generateTimestampId() { | |||
| protected String generateTimestampId() { | |||
There was a problem hiding this comment.
This was changed so that the NewCommand subclass could gain access to a timestamp formatting that was already in existence. I was trying to maintain some kind of consistency here since since timestamp is a String and not a Date or something more appropriate.
| @@ -238,6 +260,17 @@ protected File existingEnvironmentFile() { | |||
| return envFile; | |||
| } | |||
|
|
|||
| protected Properties environmentProperties() { | |||
There was a problem hiding this comment.
a convenience override.
| @@ -315,6 +348,15 @@ protected MigrationLoader getMigrationLoader() { | |||
| : new FileMigrationLoader(paths.getScriptPath(), env.getScriptCharset(), env.getVariables()); | |||
| } | |||
|
|
|||
| protected Hook createNewMigrationHook() { | |||
There was a problem hiding this comment.
Following previous code pattern.
| String before = environment().getBeforeNewHook(); | ||
| String after = environment().getAfterNewHook(); | ||
| if (before == null && after == null) { | ||
| return NoOpHook.getInstance(); |
There was a problem hiding this comment.
This follows the Null Object Pattern so that null checks don't need to be carried everywhere.
| @@ -343,6 +385,11 @@ protected MigrationHook createFileMigrationHook(String before, String beforeEach | |||
| factory.create(after)); | |||
| } | |||
|
|
|||
| protected BasicHook createBasicHook(String before, String after) { | |||
| HookScriptFactory factory = new FileHookScriptFactory(options.getPaths(), environment(), printStream); | |||
There was a problem hiding this comment.
Following previous patterns
| @@ -35,35 +44,87 @@ public void execute(String... params) { | |||
| if (paramsEmpty(params)) { | |||
There was a problem hiding this comment.
Unfortunately, I really had to overhaul this function. In a nutshell, I did the following:
- Normalized types. At the end of all this, a
Readeris required so a template can be applied. I did that by functionalizing it and still honoring options andMIGRATIONS_HOME - Functionalized the bind paramaters so it would be crystal clear what was going into it. As a bonus, I also include the extra
argssupplied to the commandline so thatnew description blab foo bazcould capturedescription blab foo baz. These get passed along and allow the new hook to operate based on commandline args. - I wanted it to be dead obvious when
hook.beforeandhook.afterwere being called and that they were literally before and after the template creation (which is what the original code did)
|
|
||
| } | ||
|
|
||
| private Reader getTemplateReader() { |
There was a problem hiding this comment.
This whole function is meant to normalize finding the reader for the template so that the calling code can seperate the what from the how what: I need to get a template. How: by checking a whole lot of config
| return templateReader; | ||
| } | ||
|
|
||
| private String getTemplateFile() throws FileNotFoundException { |
There was a problem hiding this comment.
Finds the location of the template file based on previous logic. This just functionalizes it for clarity
| private void copyDefaultTemplate(Properties variables, String filename) { | ||
| copyResourceTo("org/apache/ibatis/migration/template_migration.sql", Util.file(paths.getScriptPath(), filename), | ||
| variables); | ||
| private Map<String, Object> createBinding(String[] params) { |
There was a problem hiding this comment.
functionalizes the creation of all binding properties for clarity sake. By functionalizing it the code essentially self documents.
| /** | ||
| * @author cbongiorno on 12/29/17. | ||
| */ | ||
| public class BasicHook implements Hook { |
There was a problem hiding this comment.
Simply the base implementation of a hook. There is only before/after
| * @author cbongiorno on 12/29/17. | ||
| */ | ||
| public class BasicHook implements Hook { | ||
| protected static final NoOpHookScript NO_OP = NoOpHookScript.getInstance(); |
There was a problem hiding this comment.
Null Object Pattern. I am pretty active in design patterns :)
| } | ||
|
|
||
| @Override | ||
| public void before(Map<String, Object> bindingMap) { |
There was a problem hiding this comment.
I debated having Hook.before and Hook.after return values. This would allow for immutable execution like:
Change c = newHook.before(params); // instead of modifying change inside the binding
Testing that now.
| this.afterEachScript = afterEachScript; | ||
| this.afterScript = afterScript; | ||
| super(beforeScript, afterScript); | ||
| this.beforeEachScript = beforeEachScript == null ? NO_OP : beforeEachScript; |
There was a problem hiding this comment.
Taking advantage of previous code
| beforeScript.execute(bindingMap); | ||
| } | ||
| public FileMigrationHook(HookScript beforeScript, HookScript afterScript) { | ||
| this(beforeScript, NO_OP, NO_OP, afterScript); |
There was a problem hiding this comment.
convenience override.
| if (beforeEachScript != null) { | ||
| beforeEachScript.execute(bindingMap); | ||
| } | ||
| beforeEachScript.execute(bindingMap); |
There was a problem hiding this comment.
By using a NoOp object we never have to check for null or risk and edge-case error
| * @author cbongiorno on 12/28/17. | ||
| */ | ||
| public interface Hook { | ||
| void before(Map<String, Object> bindingMap); |
There was a problem hiding this comment.
These should return a <T> so that the results of the hook can be used (or not). The use of the Hook class was kind-of a necessity because I no longer had a FileMigrationHook I had a NewHook But creating a NewHook seemed silly because it's just a dumb implementation of any hook. Hence the SimpleHook because I didn't want to have a command-specific hook when the class can be conceptually reused.
| /** | ||
| * @author cbongiorno on 12/29/17. | ||
| */ | ||
| public final class NoOpHook implements MigrationHook { |
There was a problem hiding this comment.
Been down this road. All other hook code could use this instance/object and remove their null checks but that's outside the scope of my changes
| @@ -345,8 +345,8 @@ public void useCustomTemplateWithBadPath() throws Exception { | |||
| Migrator.main(TestUtil.args("--path=" + basePath.getAbsolutePath(), "new", "test new migration")); | |||
| String output = out.getLog(); | |||
| assertEquals(4, scriptPath.list().length); | |||
| assertTrue(output.toString() | |||
There was a problem hiding this comment.
use of toString on a String is redundant
| @@ -345,8 +345,8 @@ public void useCustomTemplateWithBadPath() throws Exception { | |||
| Migrator.main(TestUtil.args("--path=" + basePath.getAbsolutePath(), "new", "test new migration")); | |||
| String output = out.getLog(); | |||
| assertEquals(4, scriptPath.list().length); | |||
| assertTrue(output.toString() | |||
| .contains("Your migrations configuration did not find your custom template. Using the default template.")); | |||
| assertTrue(output.contains("Your migrations configuration did not find your custom template")); | |||
There was a problem hiding this comment.
Reformatted for readability
|
|
||
| out.clearLog(); | ||
| System.exit(0); | ||
| } | ||
|
|
||
| private void newHook() throws Exception { |
There was a problem hiding this comment.
Following previous testing pattern
| @@ -89,5 +89,6 @@ hook_after_each_up=sql:InsertWorklog.sql:local_var1=LOCALVAR1:local_var2=LOCALVA | |||
| hook_after_up=js:PrintVar.js:_object=dog:_method=bark:_arg=ARG1:_arg=ARG2:local_var1=LOCALVAR1:local_var2=LOCALVAR2 | |||
|
|
|||
| hook_before_each_down=JavaScript:InsertWorklog.js | |||
|
|
|||
| hook_before_new=JavaScript:before_new.js | |||
There was a problem hiding this comment.
Following previous testing pattern
| */ | ||
| if (typeof println == 'undefined') | ||
| this.println = print; | ||
| println("after new change supplied " + (change !== null)) |
There was a problem hiding this comment.
Now that I look at this, the script isn't comprehensive enough. I need to test for the existence of paths and args since those are passed in with the bindings
|
I'm sorry, but I don't have enough time to continue reviewing this PR. In case you need this feature anytime soon, I would suggest you to make the necessary change on your fork. |
|
@harawata - I am a bit confused. It's an agreed feature. I took your suggestions. I didn't change the design of hooks at all. I changed the implementation mostly to allow for the divergence of While I am not in a burning hurry to have this folded in (I did, of course fork it for PR sake) this was definitely meant to be a part of the community. Having been an OSS project owner I can sympathize with the time constraints. There is one thing which I am having trouble with: Because you don't have the time to review it (which is understandable) then at some undetermined point in the future you'll have (much more) time to just reimplement the whole thing (for reasons I can only guess at)?! I have to be honest, that's really off putting and can be taken as rude. Given that a previous PR I submitted was entirely rewritten, I think that's a fair read. In #112 I was given a pointer to a link about "learning to say no" - Among those reasons I don't see the reasoning you give. I can wait for another reviewer if I know one will pick it up. I can bullet point the changes if you'd like. I already gave a line-by-line comment to really ease the review. If you can tell me what you're looking for I am ready to help. But saying you don't have time to review it but might totally rewrite it later is a strong cautionary tale about committing code here. What I found as an OSS maintainer to be a very effective (and nearly only) method of getting people involved is to make them feel valued. If I had thought from the beginning that my changes wouldn't be folded in I would not have bothered with PRs and commentary. And, I would have just done things in a manner most appealing to me. But, I took your feedback and pains to make sure it's entirely in line with original testing and tooling. I am willing to offer whatever you and the team need to get through this; should I expect that my other outstanding PRs will get the same? |
|
So? Did anyone find some time to review these changes or totally rewrite them? |
|
Any updates? |
Changing PR to use a dedicated branch on my fork instead of master. Unfortunately I can't modify the previous PR to use a different branch
Previous PR: #109
issue number #106