restapi: incorrect href for parent href

[ArtiomDivak]

If User send REST API request for ovirt-engine/api/disks/{diskid}/disksnapshots?include_active&include_template he will get this incorrect href back <parent href="/ovirt-engine/api/disks/{diskid}" id="{parentid}"/>.
Now it will get correct output that will look like this <parent href="/ovirt-engine/api/disks/{parentdiskid}/disksnapshots/{parentid}" id="{parentid}"/>.

Bug-Url: https://bugzilla.redhat.com/2013697
Signed-off-by: Artiom Divak adivak@redhat.com

[mkemel]

Additional info: Note that when 'include_template' flag is requested, and one of the disk_snapshots have a template disk as a parent (as described here https://bugzilla.redhat.com/1900597) - this template disk snapshot belongs to another disk, thus the href should be built accordingly.

Have you checked this scenario?
In this case, the parent snapshot will have another diskId, thus the URL will be incorrect.

[smelamud]

As I see, similar change is already done by b1579af

[ahadas]

As I see, similar change is already done by b1579af

right, that was for a particular snapshot and this one is for a collection of snapshots

[mkemel]

Please note, that the href of the <disk_snapshot> element is also incorrect. It's not described in the bug, but maybe makes sense to fix that as well.

[mkemel]

A bit more readable, but I still think you should do the mapper.map(disk, null) thing once for each Disk entity, and then work with a list of DiskSnapshots

[mkemel]

much better now, thanks minor comments inside I'm still not convinced that we should set the parent link to the template's image when include-template is not set - until now I only thought if it's a proper design but now I wonder if it could lead to backward compatibility issues for clients that use that link (they'll see a link to an image that is not returned). @ArtiomDivak @mkemel @bennyz @barpavel what do you think?

I think that this link is intended to fetch a specific disksnapshot. Thus returning it when include_template is not set makes even more sense - we don't have the parent image here in the returned array of disksnapshots, but here's a correct link that you can fetch it with. Also don't think that there would be any backward compatibility issues, since this href was never correct anyway.

[ahadas]

Also don't think that there would be any backward compatibility issues, since this href was never correct anyway.

@mkemel so previously when include-template was not set we set an incorrect href for the parent of the top-level volume? wasn't it empty?

[mkemel]

@mkemel so previously when include-template was not set we set an incorrect href for the parent of the top-level volume? wasn't it empty?

No, it wasn't empty. The parent ID is always there, it's just that the URL is built incorrectly.
BTW, include_template is a fairly new feature, and was added exactly as a solution to the fact that parent pointed to a disksnapshot not in the list.

[ahadas]

No, it wasn't empty. The parent ID is always there, it's just that the URL is built incorrectly.

ack, in this case, it's really a bug fix more than an enhancement that users can complain about, ok.

[ahadas]

another minor comment, besides that lgm

[ahadas]

/ost

[ArtiomDivak]

/ost

[ahadas]

@oliel , @mwperina , can you please take a look?

[ahadas]

@ArtiomDivak I changed the code in GetDiskSnapshotByImageIdQuery to be a bit more null-safe, can you please check to see it still works so we'll get it in?