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

Improve Configuration's update result reporting #87

Closed
hinca opened this issue Mar 2, 2020 · 8 comments
Closed

Improve Configuration's update result reporting #87

hinca opened this issue Mar 2, 2020 · 8 comments

Comments

@hinca
Copy link

hinca commented Mar 2, 2020

Currently, Configuration's update() method returns a boolean whether the update was successful or not. I feel there is some room for improvement here, for this method (or a relevant sibling method) to return more details about the update. Namely:

  • message/reason for the update failing (if any)
  • whether (or how many?) jars were actually updated, or there was nothing newer to get
  • what is the current version of the business application jar (The one defined in Maven's <version /> In the business app I can get it by getClass().getPackage().getImplementationVersion())

That way updates could be logged and various feedback could be provided based on the update wizard (i.e. a popup displaying that a new version is now running, possibly linking to a change log).

@hinca hinca changed the title Improve Configuration's update more thorough result reporting Improve Configuration's update result reporting Mar 2, 2020
@mordechaim
Copy link
Contributor

mordechaim commented Mar 2, 2020

Perhaps I should've done it that way in first place but now that it's not you can still easily get those info.

  • message/reason for the update failing (if any)
  • whether (or how many?) jars were actually updated, or there was nothing newer to get

You can actually get hang of this info in the update handler itself, by implementing init() to capture the UpdateContext and failed() to get the exception.

If you need these information in the bootstrap you can use dependency injection:

In the bootstrap or common module:

public interface UpdateInfo {
    UpdateContext getUpdateContext();
    Exception getException();
}

In the bootstrap class, add this method:

@PostInject
private void getInfo(UpdateInfo info) {
    this.info = info;
}

Make your bootstrap class implement Injectible and your update handler implement UpdateInfo and use the overload of update() that takes an injectible.

config.update(this);

There are many possible ways of doing it but this allows you to not depend on the actual implementation of your update handler.

To check if the config is outdated, call config.requiresUpdate() or you can call it for individual FileMetadatas.

  • what is the current version of the business application jar

There's no way I can define a file version without assumptions, you can put this information in the comment attribute of a <file> in the config. Or you could use the same dependency injection solution as above, but for the launch() method.

@mordechaim
Copy link
Contributor

mordechaim commented Mar 3, 2020

@hinca did you manage to solve your problem?

@hinca
Copy link
Author

hinca commented Mar 3, 2020

Partially: config.requiresUpdate() works fine, I just did not notice it before. The version method with comments will work, although I would rather rely on the jar file. Will try to achieve that by overloading something and reading it from the package instead of using the comments.

The inject method getInfo gets triggered when I use the injectable update method, but the UpdateInfo info argument is null:
@PostInject private void getInfo(UpdateInfo info) { this.info = info; }

This could have to do with me not having a custom update handler, is that something I need to use for this purpose? How do I call the update with both the update handler and the injectable?

@mordechaim
Copy link
Contributor

mordechaim commented Mar 3, 2020

is that something I need to use for this purpose?

Yes, because the default handler isn't configured to pass you that info. Unless if you want you can change the injected method call to:

@PostInject
private void getHandler(DefaultUpdateHandler handler) {
}

And then use reflection to get the context field; I'd recommend you against this approach though.

How do I call the update with both the update handler and the injectable

I assume you try to use the update overload that takes an UpdateHandler? In that case you can't use injectable but you can still get all those info by directly reading that information from the instance itself.

Injectable can be used when the handler gets loaded by the service mechanism or by listing it in the config under <provider updateHandler=com.MyClass />

Perhaps tell me your general setup of the bootstrap so I can try to help.

@mordechaim
Copy link
Contributor

mordechaim commented Mar 3, 2020

I'm considering actually returning a report object instead of boolean for the next release. What information do your think should it contain?

@hinca
Copy link
Author

hinca commented Mar 3, 2020

Perhaps a slight overengineering, but I'm thinking along the lines of:

boolean success;
int amountUpdated;
List<FileMetadata?>; // (group, name, old version, new version etc)
String version; // of the "business" app jar
String message;
UpdateException exception; // (using a generic top level Exception is not great practice)

Thanks for the advice, I'll get back to implementing a custom update handler, presumably I should be able to get my info there. Will update later this week.

@mordechaim
Copy link
Contributor

mordechaim commented Mar 3, 2020

success could be as simple as checking if the exception is null.
amountUpdated is the same files.size()
version as I said is outside my scope, but why not use a config property for this (or comment, if you need a per-file property)
message I'm not sure what it should represent.

(using a generic top level Exception is not great practice)

JavaFX Task does it that way.

@hinca
Copy link
Author

hinca commented Mar 5, 2020

Okay. :) For now I decided just to use the requiresUpdate() and config property, I don't really need the amounts and other stuff.

As for the generic exceptions... I'm not a fan, it's a CWE and in breach of SEI CERT Oracle Coding Standard for Java anyway. But suppose it won't ruin my dinner. :)

http://cwe.mitre.org/data/definitions/397.html

https://wiki.sei.cmu.edu/confluence/display/java/ERR07-J.+Do+not+throw+RuntimeException%2C+Exception%2C+or+Throwable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants