Implementation of Client App

[Pa3u3u]

I have finished command-line handling for our application and I've tested it with the excursions-list command (so you only need to implement add, get and delete).

Since the Program class was extremely bloated when I finished, I have split the application into the following packages:

• c.m.p.s - Program and MainOptions,
• c.m.p.s.cli - configuration,
• c.m.p.s.cli.handlers - handlers for each command,
• c.m.p.s.cli.rest - REST clients, they do not print anything on output.

Since almost 90% of command-line handling is already done, it will be easy to implement all other methods. You essentially only need to:

1. implement your REST client,
2. implement handlers for each command (see ExcursionsListHandler),
3. register handlers in handlers map in surrealtravel.xml.

I will add documentation for necessary things later today.

Also, if you are using Linux (or Windows with Perl), I have provided two scripts that will simplify running the code, ie.:

$ ./run --help          # shows the help
$ ./run excursions-list # lists the excursions

The run-verbose will also display a lots of maven crap. The latter command should show something like this:

======================================================================
Id  Description              Destination  Date        Duration  Price   
======================================================================
 1  Best of war              Afgánistán   16-12-2014         2  150.00  
 2  Best of army stock       Afgánistán   16-12-2014         5   10.00  
 3  Best places on cemetary  Afgánistán   16-12-2014         1    5.00  
 4  Battle of Five Armies    Erebor       20-11-2941         2  500.00  
 5  Council of Elrond        Rivendell    25-11-3018         1  150.00  
 6  Destruction of Isengard  Isengard     02-04-3019         1  400.00  
 7  Battle of Hornburg       Helm's Deep  03-04-3019         1  350.00  
 8  Mt Doom Excursion        Mordor       14-04-3019         3  200.00  
 9  Downfall of Barad-dûr    Mordor       25-04-3019         2  300.00  
10  Best of war              Afgánistán   21-07-2014         2    0.00  
======================================================================

[Pa3u3u]

I have updated the previous commend to reflect the changes I did today. Now you can implement your stuff :smiley:

[Pa3u3u]

One more thing, in order to make the interface uniform, I suppose we both need to follow the same design. I suppose we should do this:

• list: no special options (already implemented),
• add: required options for each required attributes and optional options for optional attributes,
• get: required option --id,
• delete: required option --id,
• update: required option --id and optional options for each attribute that can be changed.

After successfull add and update you should print the affected entity (you can extract the code from ExcursionsListHandler that prints it as a table, since there is no usable toString() method... :smiley:

i.e. excursions-update --id 2 --description "Foobar" --duration 2 will change the description and duration of excursion with ID 2, excursions-update --id 3 will show error (user wanted to update something, but did not specify what)

[FiXerCz]

I am finally at PC and not studying for sth, so I will take a look at it now.

[FiXerCz]

Is it possible to run the jar file in the "target" directory from the command line and pass to it our arguments? I cannot figure it out. So far I am running custom maven goals (exec:java -Dexec.args="trips-list" and similar) to test the app (i dont have perl so I am running it directly from netbeans instead of "run" scripts). Edit: Nevermind, I have solution - I just store custom maven goals (for list, get, add etc) and run those.

[Pa3u3u]

I asked about that on the tutorials, you need to run the program via maven. There is the maven-plugin added to the jar, so you don't need to specify Dexec.mainClass, but you unfortunately need to specify extra arguments in Dexec.args. There is, I'm afraid, no way around it. (You can also specify (= hard-wire) the arguments in the plugin, but that makes no sense :smiley:).

2014-12-17 17:50 GMT+01:00 FiXerCz notifications@github.com:

Is it possible to run the jar file in the "target" directory from the command line and pass to it our arguments? I cannot figure it out. So far I am running custom maven goals (exec:java -Dexec.args="trips-list" and similar) to test the app (i dont have perl so I am running it directly from netbeans instead of "run" scripts).

— Reply to this email directly or view it on GitHub https://github.com/Cweorth/Surreal-Travel/issues/34#issuecomment-67352790 .

[FiXerCz]

Thanks for reply :)

I am currently struggling with adding arguments to custom CommandHandler implementations. I have already updated my client to be able to retrieve single excursion by id, created new handler (and registered it in MainOptions (@SubCommand) and application context). No problem there.

But I cannot figure out, how to add "--id" option to my handler. I have the following (long id attribute, setter, getter + @Option): http://pastebin.com/cNrLZ9Q7

According to log, id is set to 0 (java default value, but in maven goal I have set --id 1) and my argument is not proccessed.

From what I can see, MainOptions.java (all basic program options) are registered with CmdLineParser in Main.java. But our custom command handlers are not registered anywhere. Is that the problem or am I doing sth else wrong (very high chances of that)? :)

[Pa3u3u]

You also need to register the handler in the surrealtravel.xml file and more importantly (that's something I should specify somewhere :smiley: ): subcommand options must be specified after the subcommand, and main options need to preceed the subcommand. That's the way args4j handles them. So, $ program -a subcommand -b will give a to the main handler and b to the subcommand. Also, try to move the annotation from the setter to the private field (perhaps it will work then?)

[FiXerCz]

Moving annotations does not help. I had it there at first.

I have in my app. context following (not sure if that is enough):

<bean name="handlers" class="java.util.EnumMap">
        <constructor-arg>
            <map key-type="cz.muni.pa165.surrealtravel.Command" value-type="cz.muni.pa165.surrealtravel.cli.handlers.CommandHandler">
                <entry key="EXCURSIONS_LIST" value-ref="excursionsListHandler" />
                <entry key="EXCURSIONS_GET"  value-ref="excursionsGetHandler" />
                <entry key="TRIPS_LIST"      value-ref="tripsListHandler" />
            </map>
        </constructor-arg>
</bean>

MainOptions.java have this. Am I missing some options here?

@Argument(index = 0, required = true, metaVar = "command", handler = SubCommandHandler.class,
            usage = "main command")
    @SubCommands({
        @SubCommand(name = "excursions-list", impl = ExcursionsListHandler.class),
        @SubCommand(name = "excursions-get",  impl = ExcursionsGetHandler.class),
        @SubCommand(name = "trips-list",      impl = TripsListHandler.class)
    })   
private CommandHandler cmd;

I have also added EXCURSIONS_GET to Command enum.

Did I miss sth?

i could make a commit if that would make it clearer, but I try to avoid commiting sth, that does not work :)

[Pa3u3u]

Well, I'm currently packing my things for I'm leaving to Ficoland in two hours, and I don't have time to make wild guesses. Commit it so I can look at it as whole

[FiXerCz]

Ok, have a safe journey ;)

[Pa3u3u]

Thank you. The problem is probably with args4j (it lacks documentation about subcommands, so I only made this work with {excursions,trips}-list by accident :smiley:) It simply does not instantiate the cmdfield of the MainOptions for some reason. I have rewritten the line 138 in Program.java from

handler.run(options);

to

options.getCmd().run(options);

which should have fixed the problem... but it didn't, because cmd will not get instantiated. I don't know how to solve it right now. I have about one hour spare time, so I might look into that ... or I will look into that in the bus if I take some strong drugs :smiley:

Edit: All af the above (except Thank you) is a horseshit. I did not read the log properly. The problem is in line 36 of your handler, where the client is null because it did not get autowired for some reason. I will commit the change in the Program, perhaps you will be able to fix the autowiring problem.

[FiXerCz]

Yeah, I have googled for some solid documentation and found squat..

There is also addOption(Setter, Option) method for CmdLineParser parser object. I am trying to look into that. It would be dirty as hell but we could make this object accessible (singleton bean perhaps, if that would even be possible to do) from our Handler classes. We could then add options directly to parser.

But this method might haunt us in our sleep :)

[Pa3u3u]

Read the Edit part. The problem with is with the Spring and the @Autowire now, I fixed the problem with args4j by the last push (about 15 minutes ago).

[FiXerCz]

Wicked, Your last comment was loaded via github automatically but that edit part was hidden until I refreshed the page. 'miscommunicated, sorry :)

[Pa3u3u]

I know where the problem is. The REST clients get injected into handler beans, but the one that args4j creates is not a bean (it is created AFTER the annotation processor does the injection), hence it does not have the client injected. This sucks, but I think I know how to fix this. Perhaps I will make it to the bus departure :smiley:

[FiXerCz]

Nice. I have gone through my entire log and seen line after line "restExcursionClient bean prepared for autowiring,.. autowired.." and then nullpointerexception that client is null. The only thing that came to my mind was "what the f**ck?" :D

[Pa3u3u]

I thought it could be fixed by autowiring clients as static properties, but it does not work. I think I will not be able to fix it today, because I have to go now, so ... for now, try a workaround: create static fields in AppConfig for the client with getters and setters, and in the Program.java after the context initialization set this property via context.getBean(...). It's a nasty workaround because it essentially makes Spring useless, so I will have to fix it later, but it should suffice.

[FiXerCz]

Yep, that works for now. And the "--id X" is also now recognized :)

[Pa3u3u]

Can you please push your newest version, so I could either find a solution to our previous problem or get rid of Spring completely? Thanks

[FiXerCz]

Sure, I did not know I am supposed to push that temporary fix. Will do it asap.

[Pa3u3u]

You weren't, but then I realized it would cause merge conflicts if I was to do something with Spring

[FiXerCz]

It's there. Edit: Modifications in the "core" are the 2 lines (with //temp) in AppConfig.java and Program.java.

[FiXerCz]

Working with undocumented args4j is joy XD

I just implemented handlers for @Option annotation for Date and BigDecimal types. Will push them as soon as I test them a bit more thoroughly..

[Pa3u3u]

All right. These option handlers would be handy for me too. I, on the other hand, tried several ways to fix the Spring Beans injection, the only that worked was when I did not use them :smiley: So I removed Beans and moved static initialization to AppConfig (though it should have a more proper name).

I will not push it, because I made some changes in your code as well (removed annotations) and it would cause conflicts, so I will wait until you push your version and I will merge them tomorrow.

So far, your code looks good. Perhaps you could remove the original code that prints the table in ExcursionsListHandler and replace it by your CLITableExcursion.createTable?

[FiXerCz]

Okay, I will do that. I have more changes (in my code, then the classic registering of handlers in applicationContext and MainCommands).

Can I commit all those things? Won't there be any messy conflicts for you?

I rather ask before I commit any more extensive code changes.

Edit: And quick question. When editing excursion, should I require user to enter all neccessary data (like when adding) or should I allow user to enter eg. just ID (to identify excursion) and then DESTINATION (this is what user wants to change) and then get other excursion attributes by loading excursion data from the database? This explanation is a bit heavy handed, I hope you can get what I mean.. Sorry, it is already late :)

Edit 2: Nevermind my question in Edit, I found the answer in your 3rd post in the topic. Like I said, it is late :))

[Pa3u3u]

Yeah, sure, go ahead

2014-12-18 23:07 GMT+01:00 FiXerCz notifications@github.com:

Okay, I will do that. I have more changes (in my code, then the classic registering of handlers in applicationContext and MainCommands).

Can I commit all those things? Won't there be any messy conflicts for you?

I rather ask before I commit any more extensive code changes.

— Reply to this email directly or view it on GitHub https://github.com/Cweorth/Surreal-Travel/issues/34#issuecomment-67565516 .

[FiXerCz]

Yeah, sorry, I already did it today after midnight :) I am on my way home right now and did not want to slow you down as I won' t be able to commit anything 'till noon.

[Pa3u3u]

Well, you did not slow me down, because I was off in Košice wasting time with bureaucrats :smiley: But now I'm finally going to finish it.

[FiXerCz]

Howdy, I have finished completely excursion related code. I need to commit all Excursion*Handler.java (Can I also change LIST handler? I just added logging and exception catching. You are the original author, but as it is excursion related code, I think it would be ok :) ) files and RestExcursionClient.java. All files have been pulled before changing to contain changes made by you in latest commit. Is it ok? No local changes on your side?

Btw good work overally on the whole CLI client. It is very easy to work with the code and I think the final outcome will be more than satisfactory.

Most of the code for excursions will probably be applicable in some form on Trip, so I hope you can use some of it and it will make the rest of coding a bit easier.

Now I will take a look on my other code (previous checkpoints) to see if it meets requirements and java conventions.

[Pa3u3u]

You can commit your code. I'm implementing the Trip methods, but now I'm revising the RestControllers, because when something goes wrong, the server returns 500 Internal Server Error, which is wrong, it should return either 404 Not Found when entity is not found or 400 Bad Request when some entity is invalid.

I'm doing it myself, because it's an easy change - new exception annotated with @ResponseStatus thrown instead of IllegalArgumentException or ObjectNotFoundException (whose idea was that btw? :smiley: )

About the output format, in your *Add* and *Edit* handler, please change - The following excursion was added >> to The ... added: (ie. no - at the beginning and : instead of >>)

In *Get*, I don't think it's necessary to print the message The following... at all, the user asks for a excursion; either he gets it or an error is shown.

In general, do you need to catch exceptions in handlers? These are caught in the Program.java in order to have handlers as small as possible (but I can change the label to OPERATION FAILED if that's what you want to print :smiley: )

[FiXerCz]

I just thought it would be a bit more clear for user to know where the error happened. Any time something goes wrong CLI returns "An error occured while accessing REST api: 500 Internal Server Error For the details, please see the log file." If user wants to delete excursion and inputs bad id, it seems that such a message is quite general.

I can remove checking in handlers, I don't really care, to be honest :) Either way some error checking will be there..

[kiclus]

I have in ilegalargument exception in deleting bad id in trip and pritn the mesage to error log too maybe this is corect way.... What about the dont used method in servicelayer I think I will delete it now?

EDIT: Now I see in @Cweorth responce up it wasnt the best idea

[Pa3u3u]

I see. I will be more specific then: the message itself is not that bad, but when you return from handler after that, the program will return 0. If you throw an exception, the program will return non-zero, which is what we want. The problem is that you should not call System.exit([code]) in handler, so, the correct way is to let the exception from the client pass on.

BUT, the change I'm working on right now will allow us to check for specific status codes and throw specialized RESTAccessExceptions from clients instead of the one that's there now (500...). For instance, if I try to delete a trip and the response is 400, instead of Bad Request I can throw an exception saying that the trip cannot be deleted because of some dependency. Also, I will change the error message to OPERATION FAILED.

So, please do remove those exception checks, push your code and I will then push my changes.

(To further clarification: output like "NO EXCURSIONS FOUND" is OK, because the response was 200, but no other output would be shown and user would not know if it's wrong or not).

@Kiclus: I thought that IllegalArgumentException was fine as well, but it turns out that it will return 500 instead of 400 or 404; Javassist's ObjectNotFoundException was a bad idea from the start, it does not mean what you think it means :smiley:

Please, do not modify anything in the code right now.

[Pa3u3u]

I got the status codes from this link. I did not implement all of them (some do not apply to our project), so I give you a table of what codes to expect:

Method	Possible return codes
GET	200, 404
POST	200, 400, 404
PUT	200, 400, (404 - only TripRestController)
DELETE	200, 404

[Pa3u3u]

Mmm, how did you implement DELETE method? I'm struggling with the fact that template.delete does not return anything and template.exchange does not work when server returns an error.

[FiXerCz]

I did, Sorry for the wait. I did not receive email from github about new post... I will push it now, including the delete method (I used exchange with "null" as a request object and that seems to work).

[FiXerCz]

It is pushed. I also fixed my previous code and code I was working on.

I removed the exception catching from command handlers and kept only printing variations of "not found" messages. RestExcursionClient should be complete.

[FiXerCz]

I made lot of code shifting and changing in previous 15 minutes. I might have forgotten to change sth, I will do that l8r on.

Even though my code seems to work and I have already fixed my code from prev. checkpoints, if there will be something needed from me (or I did not do sth right), pls let me know ASAP. I cannot be at PC till deadline, I have other quite serious matters I need to deal with tonight. It is bother, but I cannot change that..

[Pa3u3u]

Okay. Well, I checked your client and handlers, only two things: 1) Add:58, Delete:37, Edit:54, Get:37 contain condition

if (excursion == null) {
    // print some error
}

these errors will never be displayed, because the template would throw an exception (status would be other than 2**) rather than returning null. If you have some time left, you can implement return code "handlers" in the client like I did (see RestTripClient, it essentially parses return code from the underlaying exception that template has thrown and rethrows it with better message. It can be done cleaner, but I have no time for that :smiley:). 2) in your client, remove all blocks

if (!response.hasBody()) {
    throw new RESTAccessException(response.getStatusCode().toString());
}

as above, the inside of this body will never be called (I thought it would when I wrote it first, but I was wrong. Mea culpa).

Apart from that, I think that the client is complete. I will start checking the code.

[FiXerCz]

Ok, thank you for notes. I will do it immediately.

I did not know if client methods in handlers can return null and at the same time return non error status. Thanks for clearing that up :)