[Task] Allow ArchivedRecordings to include target field in archivedRecording notifications

[maxcao13]

Backend implementation of cryostatio/cryostat-web#430

[maxcao13]

@andrewazores It feels like there's a couple ways to go about this but I'm not sure what is the most optimal or if any are.

1. I was thinking that the targetId would be included in the metadata by just adding a field, but that's likely not what metadata is for.
2. Then I thought it may be part of ArchivedRecordingInfo, but we don't store that anywhere.
3. Then maybe I thought I could get the targetId by searching the recordingMetadataMap inside the RecordingMetadataManager by searching by keys for a key with the same recordingName in the pair<targetId, recordingName> as the <ARCHIVES, filename> (where we parse the fileName for the recordingName) but that seems too complicated for this.

Am I missing something here?

[andrewazores]

I think you're diving really deep into specific files already, like RecordingMetadataManager - I'm not sure how you got to that file, but at a glance I don't think it relates to the problem at hand.

The root of the problem is that there is a WebSocket notification that gets emitted when ex. an archived recording is deleted from disk, but that notification only includes information about the name of the file and not the directory that it was stored within. Have you determined where in the codebase this notification is emitted? Once you know what it is that sends the notification, you can figure out if that class or anything around it already knows about the directory that the file was stored in.

[maxcao13]

The notification is being emitted from deleteRecording inside RecordingArchiveHelper when the notificationFactory is creating a notification with
notificationFactory .createBuilder() .metaCategory(DELETE_NOTIFICATION_CATEGORY) .metaType(HttpMimeType.JSON) .message(Map.of("recording", archivedRecordingInfo)) .build() .send();

and I just need to add an extra map entry with ("target", something)

I misunderstood the problem I think then, because I just need the directory it was stored under? I thought I needed the targetId like connectionDescriptor.targetId()

Then all I would need is ("target", parentPath.toString()) right?

[andrewazores]

Right, RecordingArchiveHelper is the correct class.

because I just need the directory it was stored under? I thought I needed the targetId like connectionDescriptor.targetId()

Are you sure those aren't already the same thing? ;-)

https://github.com/cryostatio/cryostat/blob/fd7a2d85c5885c317c9080cc937ee6da59641da0/src/main/java/io/cryostat/recordings/RecordingArchiveHelper.java#L266

[maxcao13]

            byte[] decoded = base32.decode(parentPath.toString());
            String decodedTargetId = new String(decoded, StandardCharsets.UTF_8);

For some reason, I'm having trouble decoding the base32 String back into readable characters, when I do this I get a bunch of
�6= ���T����͕�٥��驵��ɵ�輼������ɵ�輽����Ʉ����Ľ���ɵ`

but I'm supposed to get /service:jmx:rmi:///jndi/rmi://fedora:9091/jmxrmi

EDIT: It seems like maybe the forward slash maybe why "/" i will try later

[andrewazores]

$ tree archive/
archive/
├── file-uploads
└── ONSXE5TJMNSTU2TNPA5HE3LJHIXS6L3KNZSGSL3SNVUTULZPMNZHS33TORQXIORZGA4TCL3KNV4HE3LJ
    └── io-cryostat-Cryostat_foo_20220609T205456Z.jfr

echo -n ONSXE5TJMNSTU2TNPA5HE3LJHIXS6L3KNZSGSL3SNVUTULZPMNZHS33TORQXIORZGA4TCL3KNV4HE3LJ | base32 -d
service:jmx:rmi:///jndi/rmi://cryostat:9091/jmxrmi

echo -n service:jmx:rmi:///jndi/rmi://cryostat:9091/jmxrmi | base32
ONSXE5TJMNSTU2TNPA5HE3LJHIXS6L3KNZSGSL3SNVUTULZPMNZHS33TORQXIORZGA4TCL3KNV4H
E3LJ

Working as expected from an external perspective. Is parentPath.toString() perhaps the wrong value? It should just be the last part of the path, not the entire absolute path, for example.

but I'm supposed to get /service:jmx:rmi:///jndi/rmi://fedora:9091/jmxrmi

I don't think the leading / should be part of the targetId/URL actually.

[maxcao13]

Yeah, it was giving the absolute path value and then when the decode function acts on that, it messes it up because for some reason it doesn't like forward slashes to decode. So I have to use parentPath().getFilename().toString().

I don't think the leading / should be part of the targetId/URL actually.

I left that in there accidently because I was copy and pasting.

Anyways it works now thanks for the help!

[maxcao13]

I'm trying to fix the tests for the class and I get a null pointer exception like
[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 4.429 s <<< FAILURE! - in io.cryostat.recordings.RecordingArchiveHelperTest [ERROR] shouldDeleteRecording Time elapsed: 4.407 s <<< ERROR! java.lang.NullPointerException at io.cryostat.recordings.RecordingArchiveHelper.getCachedReportPath(RecordingArchiveHelper.java:265) at io.cryostat.recordings.RecordingArchiveHelper.deleteReport(RecordingArchiveHelper.java:255) at io.cryostat.recordings.RecordingArchiveHelper.deleteRecording(RecordingArchiveHelper.java:234) at io.cryostat.recordings.RecordingArchiveHelperTest.shouldDeleteRecording(RecordingArchiveHelperTest.java:799)
in the test at https://github.com/cryostatio/cryostat/blob/fd7a2d85c5885c317c9080cc937ee6da59641da0/src/test/java/io/cryostat/recordings/RecordingArchiveHelperTest.java#L751

I've tried to add a line of Mockito.when(archivedRecordingsPath.resolve(recordingName + ".report.html")) .thenReturn(Path.of(recordingName + ".report.html"));
because it seems to throw the exception when doing getCachedReportPath.

This test also doesn't use a lot of Mocks like the others, I've been stuck on this and I'm not sure what to do here.

EDIT: I fixed it.

[andrewazores]

@maxcao13 what was the root problem and the fix?

[maxcao13]

I think it was just cause I didn't have mock behaviour for archivedRecordingsReportPath.resolve() as well as for archivedRecordingsReportPath.absolutePath() so when getCachedReportPath gets called it gets a null pointer.

In the example above, I accidently use archivedRecordingsPath instead and that's why the mock behaviour doesn't happen.