API: Provide absolute link to record page

[maccabeelevine]

A client application could just determine this based on the base URL of the API itself. But for GenAI clients, it's helpful to provide a full URL in the response so that it can recognize the URL as such and do logical things like present it to the user.

[maccabeelevine]

Trying to figure out how to write a useful unit test for this. RecordFormatterTest right now is not accurately testing recordPage (I think) since it's somehow returning an absolute, if fake, URL.

[maccabeelevine]

Trying to figure out how to write a useful unit test for this. RecordFormatterTest right now is not accurately testing recordPage (I think) since it's somehow returning an absolute, if fake, URL.

Wrote some tests, verbose but it works.

[demiankatz]

I see that you fixed the problem I highlighted while I was busy rambling about it -- but most of my rambling remains relevant even after the bug is fixed. See below:

[demiankatz]

Thanks, @maccabeelevine! See below for some further suggestions.

I'd also be interested to hear what @EreMaijala thinks about the overall problem. I'm not sure that it's very intuitive that the default link is relative rather than absolute. We may be stuck with it for back-compatibility at this point, but he might have stronger opinions than me about how things should be named and documented.

[EreMaijala]

I'm not a huge fan of stitching URL's together from multiple sources. It would be nice if RecordLinker allowed passing the 'force_canonical' option to router. I suppose this is okay for the time being. Alternatively, it would be nice if there was an option to define the base url to be used for record links. This would enable the field to work also when API is served from a different base address than VuFind UI. Anyway, since I'm not sure if it's just us having this issue, I'm approving this as is. :)

[demiankatz]

Thanks, @maccabeelevine, this is looking great overall! See below for some (mostly nitpicky) comments and suggestions.

There are also a couple of bigger-picture things I'm still wondering about:

1.) Maybe we should split this in half and introduce the ServerUrlHelper in a separate PR from the rest. That's especially valuable if we decide to target 12.0 for the API changes, since it would allow us to introduce the helper in 11.1 in case it's useful in other contexts. I'm happy to do the work of splitting this apart and submitting a separate PR for your approval if that would be helpful.

2.) It still feels a bit hacky to have two different record URL fields for relative and absolute versions of the same thing. I wonder if we'd be better off adding a query parameter for record URL format or something (though maybe complicating the input is not a good answer). If we do keep the separate values, maybe we should consider deprecating the current record URL field in favor of choosing explicit relative or absolute fields for greater clarity. Or maybe the answer is to make the record URL behavior configurable as @EreMaijala suggests, so we could have "relative URL" as default behavior, but make it possible to override it with either an explicitly configured base URL or an autodetected one.

I don't think there's much work remaining to get the code in good shape -- but it seems worth discussing a little further to be sure we all feel good about the approach we've chosen!

[demiankatz]

Should we consider making this optional and skipping the field if it is unset, for better back-compatibility with 11.0? If not, maybe we should target this whole thing for 12.0.

[maccabeelevine]

Should we consider making this optional and skipping the field if it is unset, for better back-compatibility with 11.0?

Skipping the field, meaning returning an empty string from getRecordPageAbsoluteLink? Or throwing an exception?

[demiankatz]

I was thinking of omitting the field from the response, but in terms of implementation details, I'm not familiar enough with the code to know how you would actually do that -- whether with an empty or null return or some other way. Maybe it's not practical. I could live with throwing an exception if that's the easiest approach!

[maccabeelevine]

I implemented the exception. I don't like the idea of returning an empty or null value here, since the client may be an LLM and needs clear indication that something went wrong, so it reports it and the dev can fix the query.

[maccabeelevine]

1.) Maybe we should split this in half and introduce the ServerUrlHelper in a separate PR from the rest. That's especially valuable if we decide to target 12.0 for the API changes, since it would allow us to introduce the helper in 11.1 in case it's useful in other contexts. I'm happy to do the work of splitting this apart and submitting a separate PR for your approval if that would be helpful.

Done in #4983.

2.) It still feels a bit hacky to have two different record URL fields for relative and absolute versions of the same thing. I wonder if we'd be better off adding a query parameter for record URL format or something (though maybe complicating the input is not a good answer). If we do keep the separate values, maybe we should consider deprecating the current record URL field in favor of choosing explicit relative or absolute fields for greater clarity. Or maybe the answer is to make the record URL behavior configurable as @EreMaijala suggests, so we could have "relative URL" as default behavior, but make it possible to override it with either an explicitly configured base URL or an autodetected one.

I added an explicit method for getting the relative URL, and deprecated the original method. I agree that's a reasonable step.

It's still hacky, and normally I'd just add a query parameter to choose between absolute or relative, but I don't see how that would work with the existing SearchApiRecordFields.yaml. And I don't think a global config is the right way to go, as whatever use cases existed for a relative URL may still be there in addition to this MCP-driven need for an absolute path. Maybe I'm missing what you two have in mind here.

[demiankatz]

1.) Maybe we should split this in half and introduce the ServerUrlHelper in a separate PR from the rest. That's especially valuable if we decide to target 12.0 for the API changes, since it would allow us to introduce the helper in 11.1 in case it's useful in other contexts. I'm happy to do the work of splitting this apart and submitting a separate PR for your approval if that would be helpful.

Done in #4983.

Thanks -- I've merged that and resolved conflicts with this branch!

I added an explicit method for getting the relative URL, and deprecated the original method. I agree that's a reasonable step.

It's still hacky, and normally I'd just add a query parameter to choose between absolute or relative, but I don't see how that would work with the existing SearchApiRecordFields.yaml. And I don't think a global config is the right way to go, as whatever use cases existed for a relative URL may still be there in addition to this MCP-driven need for an absolute path. Maybe I'm missing what you two have in mind here.

Maybe the only other step is to add a deprecation note to the field description in the YAML as well so we can potentially remove it in the future. I'm not sure if there's a more formal way of deprecating fields in the spec.

[maccabeelevine]

Maybe the only other step is to add a deprecation note to the field description in the YAML as well so we can potentially remove it in the future. I'm not sure if there's a more formal way of deprecating fields in the spec.

Good call, I was thinking of that but didn't do it. Done now as cleanly as I can guess, I don't see anything formally defined. In theory I could make the deprecation an actual field, and spit out a log warning, but I don't think we do that anywhere else.

[demiankatz]

Thanks, @maccabeelevine -- see below for a couple more minor suggestions.

[demiankatz]

I was actually thinking of putting this deprecation note in the description field as well so it shows in the published spec.

[maccabeelevine]

Moved the detail to 'description' field.