Remove event specifier string syntax

[hareetd]

Remove all event specifier string syntax. Eliminates user confusion and encourages use of event templates which are a better alternative because of the reasons stated in #480.

[andrewazores]

This is a good start, although this particular issue is deeper than just COMMANDS.md - the event specifier syntax is also something implemented in code. Here's an example:

https://github.com/cryostatio/cryostat/blob/main/src/main/java/io/cryostat/net/web/http/api/v1/TargetRecordingsPostHandler.java#L92

[andrewazores]

There are a lot of unrelated changes in this patch at the moment - lots of changes from main look like they've been reverted, possibly from the merge commit in this set. Please clean up the patch so that it only makes the specific changes you've intended to make - a git rebase -i main might help do the trick here.

Otherwise, the main relevant portion of the patch is looking good - nice work! Some unit tests will need to be updated to suit, which will also fix the failing CI build status.

Once the above are addressed then go ahead and unmark this PR as a draft. I'm not sure if I get notified when that happens so to be sure you can also leave a comment to ensure I see it.

[hareetd]

Thanks for the advice, it's much appreciated!

Everything was going fine until I rebased onto main and pushed to my remote branch, but forgot to squash my commits, so I did an interactive rebase immediately after. This resulted in my local and remote branches diverging, which I accidentally merged together. After that I did a series of reverts with a final rebase onto main, which ended up with the final commit you see above.

Unfortunately, this somehow brought in the unrelated file changes you mentioned. My understanding of Git is still fairly fuzzy so I'm not sure how to fix this.

[andrewazores]

Hmm. Now that the changes are all squashed down into a single commit you'll probably need to temporarily back out of the commit locally, fix the unintentional changes, and create a new commit containing only the changes you want to include. git reset --soft HEAD^ could be useful - this will drop the most recent commit and move all of the changes it had made into the staging area. You can then use git reset and other tools to manipulate the changes until you have it boiled down to the essentials, and then git commit it again and force-push.

[hareetd]

Looks like it worked, thanks for the help! Would you like me to take a stab at changing the test suite functionality from event strings to templates before I unmark this PR as a draft?

[andrewazores]

Cool, the patch looks much better now.

Would you like me to take a stab at changing the test suite functionality from event strings to templates before I unmark this PR as a draft?

Yes, please do. Let me know if you're unsure about any of the changes required to the unit tests, but I think the CI build logs already pointed you to where the test failure occurs, so it should hopefully be fairly straightforward to update the tests to match the new expected behaviour.

[hareetd]

Sounds good, I'll be sure to let you know if I need help.

[hareetd]

So the PR looks like it's ready to go in terms of anything related to moving from strings to templates but there's an issue (inside the same test class) with this unit test:

https://github.com/cryostatio/cryostat/blob/main/src/test/java/io/cryostat/net/web/http/api/v1/TargetRecordingsPostHandlerTest.java#L314.

It seems the test uses a Stream of Map<String, String> containing invalid recordings options as input, to test if the correct HttpStatusException for invalid options is thrown. The problem is that a NullPointerException is thrown instead by the Map class because some of the Maps have been initialized with null values, such as:

https://github.com/cryostatio/cryostat/blob/main/src/test/java/io/cryostat/net/web/http/api/v1/TargetRecordingsPostHandlerTest.java#L343

The weird thing is that this test failure goes unnoticed by the overall testing suite (i.e. mvn test/verify, Github CI checks etc.). I only see the test failure when I run the individual test inside VSCode (which could mean it's just a "me" problem?).

I tried to get past this by using an anonymous sublass to replace the Map.of("duration", null) with
new HashMap<String, String>() {{ put("duration", null); }}. This takes care of the NullPointerException with respect to Map but gets replaced instead by a NullPointerException in the MultiMap class, when the invalid options are added to the HTTP form attributes:

https://github.com/cryostatio/cryostat/blob/main/src/test/java/io/cryostat/net/web/http/api/v1/TargetRecordingsPostHandlerTest.java#L331

Even if this solution had fully worked it seems that it can result in some bad side effects as outlined here: https://stackoverflow.com/a/6802502, although I don't know if these concern us.

Is there a better way to go about this in Java? Or should I just delete the null values from the test since MultiMap already disallows null values on its own, which (I think) means at some point during actual use, a NullPointerException would be thrown if the form attributes did happen to receive a null value. If true, this implies that the invalid recording options HttpStatusException would never be thrown in the case of such input anyways.

[andrewazores]

So the PR looks like it's ready to go in terms of anything related to moving from strings to templates but there's an issue (inside the same test class) with this unit test:

https://github.com/cryostatio/cryostat/blob/main/src/test/java/io/cryostat/net/web/http/api/v1/TargetRecordingsPostHandlerTest.java#L314.

It seems the test uses a Stream of Map<String, String> containing invalid recordings options as input, to test if the correct HttpStatusException for invalid options is thrown. The problem is that a NullPointerException is thrown instead by the Map class because some of the Maps have been initialized with null values, such as:

https://github.com/cryostatio/cryostat/blob/main/src/test/java/io/cryostat/net/web/http/api/v1/TargetRecordingsPostHandlerTest.java#L343

The weird thing is that this test failure goes unnoticed by the overall testing suite (i.e. mvn test/verify, Github CI checks etc.). I only see the test failure when I run the individual test inside VSCode (which could mean it's just a "me" problem?).

That is weird. It could be a Java version difference, perhaps? Check which JDK version you have installed/configured on your system and if VSCode is following that or has its own setting.

I tried to get past this by using an anonymous sublass to replace the Map.of("duration", null) with
new HashMap<String, String>() {{ put("duration", null); }}. This takes care of the NullPointerException with respect to Map but gets replaced instead by a NullPointerException in the MultiMap class, when the invalid options are added to the HTTP form attributes:

https://github.com/cryostatio/cryostat/blob/main/src/test/java/io/cryostat/net/web/http/api/v1/TargetRecordingsPostHandlerTest.java#L331

Even if this solution had fully worked it seems that it can result in some bad side effects as outlined here: https://stackoverflow.com/a/6802502, although I don't know if these concern us.

I wouldn't worry about these concerns. This is inside of a test class, so the additional memory/storage resources used by the anonymous class are insignificant (this just very, very slightly increases the requirements for dev/testing on our local machines), and similarly for the class holding a reference to its context and blocking GC - this would only be for the duration of the test run anyway.

The new Map<>(){{ put(); }} pattern would be best avoided in actual application code, but in a unit test it's not as big of a deal. It's still much less preferable than the other ways, however.

Is there a better way to go about this in Java? Or should I just delete the null values from the test since MultiMap already disallows null values on its own, which (I think) means at some point during actual use, a NullPointerException would be thrown if the form attributes did happen to receive a null value. If true, this implies that the invalid recording options HttpStatusException would never be thrown in the case of such input anyways.

Right, it sounds like in this case the test is over-specifying some constraints which may never occur in practice. If that's true then it's safe to remove those cases from the test. ie we don't need to test what the handler does if it gets a null form value, if the form value can never be null to begin with.

To check what actually happens you could run through this case manually and insert some logging statements (or figure out how to attach a debugger inside a container) in the handler to print out the form attributes map, then make requests to this API handler and see what the form attributes map actually contains. curl/HTTPie would be useful for this. The behaviour is probably that the form attribute is either not in the map at all, or if the attribute does become a key in the map then the corresponding value is always a String (possibly empty but non-null).

[hareetd]

Thanks for the tips and suggestions. I'm busy with other onboarding tasks for the rest of the day so I'll try to run through the case manually like you suggested on Monday. Based on the result, I'll decide whether or not it's fine to delete the null testing, update my branch accordingly and notify you that the PR is ready to review.

[hareetd]

@andrewazores PR is ready for review, please and thanks.

[andrewazores]

Patch looks good, however I will refrain from approving/merging this until the corresponding -web side change is made (cryostatio/cryostat-web#224).

[andrewazores]

Good catch on the command channel implementation side - I honestly forget we still have that. It is slated for removal eventually, but in the meantime this kind of feature parity change is still a good idea.

[hareetd]

What exactly is the command channel? In this example, the AbstractRecordingCommand class is only referenced by the TargetTemplateGetHandler class, and even that's just to retrieve the class field ALL_EVENTS_TEMPLATE. I initially thought it would be extended into various subclasses but that doesn't seem to be the case.

What's also interesting is that TargetRecordingsPostHandler implements the same event handling methods that AbstractRecordingCommand does.

I looked at some of the other classes in the command folder and there doesn't seem to be a pattern in their usage as far as I can see.

[andrewazores]

The command channel is the predecessor to the current notifications channel. ContainerJFR was initially implemented as an interactive-mode utility with Commands implementing various actions a user could perform. The direct shell interface was then extended with remote network (TCP and later WebSocket) access, creating the Command Channel. Later, more of the Commands' functionalities were ported over to the HTTP API, deprecating the Commands. Most of the Commands were then fully removed, including any of the ones that performed actions to do with recordings, all of which extended the AbstractRecordingCommand. The only Command implementations left are small remnants of the original Command Channel, and are due for removal some time before the V2 release.

[hareetd]

Makes sense, thanks for the explanation!

[andrewazores]

The corresponding -web PR has been merged, so this backend change can now be merged as well. Please add a commit here that updates the web-client submodule to that commit hash:

cryostatio/cryostat-web@0042988