Bug 28201

Maybe this is close to svc/import_bib from Bug 7613

Created attachment 128360 [details] [review]
Bug 28201: Add API route to create a biblio

Example usage:

POST /api/v1/biblios
Accept: application/json

{
    "marcxml": "<record>...</record>",
    "framework_id": "FA"
}

It can return data in the same formats as GET /api/v1/biblios/:biblio_id
depending on the value of the Accept request header:
- application/json
- application/marcxml+xml
- application/marc-in-json
- application/marc
- text/plain

Test plan:
1. Try requesting this endpoint with your favorite API tool
   (I recommend https://github.com/frigus02/RESTer)
2. Run `prove t/db_dependent/api/v1/biblios/post.t`

Removing the "update" part of this bug. It should be done in another bug IMO.

I like where this is going, but I think this needs some more thinking.

In my opinion:
- The request body should include the raw record
- Content/Type should tell the controller how it should handle the raw record in terms of serialization format (i.e. application/marcxml+xml, application/marc-in-json, etc)
- The framework should be passed as a header. I propose x-koha-cataloguing-fw to ease your work.
- We still need the 'biblio_metadata.schema' attribute, put it in some header as well: x-koha-metadata-schema.

Let me know if you will have time to do it. This is a nice goal for 22.05 :-D

(In reply to Tomás Cohen Arazi from comment #4)
> I like where this is going, but I think this needs some more thinking.
> 
> In my opinion:
> - The request body should include the raw record
> - Content/Type should tell the controller how it should handle the raw
> record in terms of serialization format (i.e. application/marcxml+xml,
> application/marc-in-json, etc)

That's what I wanted to do at first, but I couldn't figure out how to pass non-json data to the API. If I remember correctly the body was always empty when trying to use it in the controller. So I opted for the easy way.
Do we have examples of Koha API routes that accept data other than JSON ?

(In reply to Julian Maurice from comment #5)
> (In reply to Tomás Cohen Arazi from comment #4)
> > I like where this is going, but I think this needs some more thinking.
> > 
> > In my opinion:
> > - The request body should include the raw record
> > - Content/Type should tell the controller how it should handle the raw
> > record in terms of serialization format (i.e. application/marcxml+xml,
> > application/marc-in-json, etc)
> 
> That's what I wanted to do at first, but I couldn't figure out how to pass
> non-json data to the API. If I remember correctly the body was always empty
> when trying to use it in the controller. So I opted for the easy way.
> Do we have examples of Koha API routes that accept data other than JSON ?

Try

"schema": { "type": "string" }

for the body parameter.

(In reply to Tomás Cohen Arazi from comment #6)
> (In reply to Julian Maurice from comment #5)
> > (In reply to Tomás Cohen Arazi from comment #4)
> > > I like where this is going, but I think this needs some more thinking.
> > > 
> > > In my opinion:
> > > - The request body should include the raw record
> > > - Content/Type should tell the controller how it should handle the raw
> > > record in terms of serialization format (i.e. application/marcxml+xml,
> > > application/marc-in-json, etc)
> > 
> > That's what I wanted to do at first, but I couldn't figure out how to pass
> > non-json data to the API. If I remember correctly the body was always empty
> > when trying to use it in the controller. So I opted for the easy way.
> > Do we have examples of Koha API routes that accept data other than JSON ?
> 
> Try
> 
> "schema": { "type": "string" }
> 
> for the body parameter.

Ohhhh that's cool. Previously, I had no luck getting the validator to accept anything but JSON, but if that works that would be awesome...

So for examples...
api/v1/swagger/paths/biblios.json
Koha/REST/V1/Biblios.pm

Looking at https://docs.mojolicious.org/Mojolicious/Plugin/DefaultHelpers#respond_to and it looks to me like Koha/REST/V1/Biblios.pm might actually be a bit inefficient. It looks like it runs every output option (e.g. "as_xml_record", "to_mij", "as_usmarc", "as_formatted") regardless of what content type is actually served to the user... might be worthwhile wrapping those method calls in anonymous functions...

But that's neither here nor there ﷐[U+1F605]﷑

(In reply to David Cook from comment #8)
> Looking at
> https://docs.mojolicious.org/Mojolicious/Plugin/DefaultHelpers#respond_to
> and it looks to me like Koha/REST/V1/Biblios.pm might actually be a bit
> inefficient. It looks like it runs every output option (e.g.
> "as_xml_record", "to_mij", "as_usmarc", "as_formatted") regardless of what
> content type is actually served to the user... might be worthwhile wrapping
> those method calls in anonymous functions...
> 
> But that's neither here nor there ﷐[U+1F605]﷑

I don't think it is working as you suggest. Not I read the docs the same way you do. Let's run it on a debugger.

(In reply to Tomás Cohen Arazi from comment #9)
> (In reply to David Cook from comment #8)
> > Looking at
> > https://docs.mojolicious.org/Mojolicious/Plugin/DefaultHelpers#respond_to
> > and it looks to me like Koha/REST/V1/Biblios.pm might actually be a bit
> > inefficient. It looks like it runs every output option (e.g.
> > "as_xml_record", "to_mij", "as_usmarc", "as_formatted") regardless of what
> > content type is actually served to the user... might be worthwhile wrapping
> > those method calls in anonymous functions...
> > 
> > But that's neither here nor there ﷐[U+1F605]﷑
> 
> I don't think it is working as you suggest. Not I read the docs the same way
> you do. Let's run it on a debugger.

There is a little error in what I said. If application/json is sent as an Accept header, then only a JSON response will be generated. However, looking at the code, if application/json is not sent in the Accept header, then we're creating a hash composed of marcxml, mij, marc, txt, and any keys with their corresponding values. That's just straight up Perl code.

That said, it is possible I'm wrong about wrapping the method calls in anonymous functions though. I don't think the method should be called until the anonymous function is run, but it could be worth checking with a debugger.

I'll open a separate report for that so I stop distracting from the real purpose of this report.

Created attachment 135058 [details] [review]
Bug 28201: Add API route to create a biblio

Example usage:

POST /api/v1/biblios
Accept: application/json

{
    "marcxml": "<record>...</record>",
    "framework_id": "FA"
}

It can return data in the same formats as GET /api/v1/biblios/:biblio_id
depending on the value of the Accept request header:
- application/json
- application/marcxml+xml
- application/marc-in-json
- application/marc
- text/plain

Test plan:
1. Try requesting this endpoint with your favorite API tool
   (I recommend https://github.com/frigus02/RESTer)
2. Run `prove t/db_dependent/api/v1/biblios/post.t`

Patch rebased on master + added the use of anonymous subroutines for respond_to.

There is no need to test with a debugger IMO. It's pretty obvious that we pass a hash to respond_to, and this hash has to be built before calling respond_to (unless some kind of dark magic is involved here :))

(In reply to Tomás Cohen Arazi from comment #4)
> I like where this is going, but I think this needs some more thinking.
> 
> In my opinion:
> - The request body should include the raw record
> - Content/Type should tell the controller how it should handle the raw
> record in terms of serialization format (i.e. application/marcxml+xml,
> application/marc-in-json, etc)
> - The framework should be passed as a header. I propose
> x-koha-cataloguing-fw to ease your work.
> - We still need the 'biblio_metadata.schema' attribute, put it in some
> header as well: x-koha-metadata-schema.

The more I think about this, the more I disagree with it.
The frameworkcode is a property of a biblio record in the same way as the MARC record. I don't see why it shouldn't be passed with the body. Especially as the response to GET /biblio/:id already returns the frameworkcode in the response body.
Actually, I think we should go in the opposite direction and pass the MARC record inside a "metadata" property. Something like this:

POST /api/v1/biblios

{
  "framework_id": "",
  "metadata": [
    {
      "format": "marcxml",
      "schema": "UNIMARC",
      "metadata": "<record>...</record>"
    }
  ]
}

It's more in line with the database structure.
And we may want to set other columns of biblio or biblioitems (for those that are not mapped to a MARC subfield)
What do you think ?

(In reply to Julian Maurice from comment #14)
> (In reply to Tomás Cohen Arazi from comment #4)
> > I like where this is going, but I think this needs some more thinking.
> > 
> > In my opinion:
> > - The request body should include the raw record
> > - Content/Type should tell the controller how it should handle the raw
> > record in terms of serialization format (i.e. application/marcxml+xml,
> > application/marc-in-json, etc)
> > - The framework should be passed as a header. I propose
> > x-koha-cataloguing-fw to ease your work.
> > - We still need the 'biblio_metadata.schema' attribute, put it in some
> > header as well: x-koha-metadata-schema.
> 
> The more I think about this, the more I disagree with it.
> The frameworkcode is a property of a biblio record in the same way as the
> MARC record. I don't see why it shouldn't be passed with the body.
> Especially as the response to GET /biblio/:id already returns the
> frameworkcode in the response body.
> Actually, I think we should go in the opposite direction and pass the MARC
> record inside a "metadata" property. Something like this:
> 
> POST /api/v1/biblios
> 
> {
>   "framework_id": "",
>   "metadata": [
>     {
>       "format": "marcxml",
>       "schema": "UNIMARC",
>       "metadata": "<record>...</record>"
>     }
>   ]
> }
> 
> It's more in line with the database structure.
> And we may want to set other columns of biblio or biblioitems (for those
> that are not mapped to a MARC subfield)
> What do you think ?

I think you got it right here. I'd say the 'metadata' attribute should always be Base64 encoded, to support binary formats as well as XML. I've seen that in other APIs. This should be better documented on the spec.

And maybe the update should be PUT /biblios/:biblio_id/metadata

What's missing here to move it out of discussion?

This was replaced by bug 31800

*** This bug has been marked as a duplicate of bug 31800 ***