Requirements

[judgej]

Based on the php generator templates, these are the main differences:

• Usr PSR-18 client to send requests.
  • PSR-18 client injected rather than Guzzle client.
  • A discovered ClientFactory could be used if no client injected.
  • The authentication built in could still be supported if desired, but with simple PSR-18 decorators.
    These would only be used at run-time if authentication details have been passed in,
    otherwise the assumption is made that authentication is already built into the supplied PSR-18 http client. See issue Strip out Authentication #2
• PSR-7 messages generated internally from an injected or discovered RequestFactory.
  • Accept injected request factory (primary usage).
  • Discover request factory (fallback usage).
• The models to be serialisable (as JSON), which would work with nested responses. I can see that some models are serialisable direct to JSON, and some are not.
• Models to be immutable. They are mutable with getters and setters now; they need public getters, protected setters, and a public with*() methods. This is opinionated, but is where we would like to go.
• Models should support magic __get() methods to access the properties. Needing to use individual getters for every property is not so easy.
• Check laravel's get_data() method (with its underlying Symfony support) can walk the nested response objects and models.
• PSR-0 and PSR-1 to be tested for (no trailing blank lines on models and API classes, for example).
• Remove a lot of duplication in the generated code.

[judgej]

The duplication in the generated code results in two problems:

• The number of lines of code is massive - it grows very quickly with the number of endpoints. This will slow down compile time.
• The duplicated code means during development - tweaking to try things out - I cannot change just one instance of some code and see how it works across a number of endpoints. The same blocks of code are literally repeated over and over, and all would need changing, which is impracticable. This makes development much slower.

The Xero APIs generated like this are going to run into the multiple tens of thousands of lines. It is going to break. The fix will be to move repeated blocks into separate functions.

[judgej]

@bradydan the link above is a live API call to the getServiceLineTypes() API. It is used easily like this:

// A plain PSR-18 client
$baseClient = Http\Factory\Discovery\HttpClient::client();

// Decorated (wrapped) with our authorisation layer for Xero:
$httpClient = $xeroPartnerClient::getClient($baseClient);

// Instantiate the generated API class with our decorated PSR-18 client:
$accountingApi= new Client\Api\AccountingApi($httpClient);

// Call the endpoint we need to access. This one takes no parameters.
$response = $accountingApi->getOrganisations();

// Look at the result:
dump($response);

The first three steps would be wrapped up into a framework service, so the accountingApi is simply created by the service whenever we need it and without fiddling on with those HTTP client layers. So we just say, "hey client, give me the stuff", and we have them through all the invisible layers of requests and stuff.

All the API stuff is hidden away, and we can then focus on the business rules of the code. That's what we are aiming at.

[bradydan]

William’s GitHub user is @wing328

[judgej]

OMG, the Xero Accounting API class is 57k lines line. 57,385 lines. One class. That's just crazy. It is probably usable, but we'll try to make some de-duplication gains where we can (but if it works, it won't be the focus).

There is another 51k lines of code making up the 109 model classes too.

Now imagine writing that code by hand - over 100,000 lines. I don't envy those even writing the OpenAPI spec. I'm NOT going to run the OpenBanking spec through this quite yet. That will be multiple times bigger.

[bradydan]

Wow. Now we get to see what a mammoth job their move to OpenAPI has been. Let’s hope the code is usable.

The Xero accounting API seems to contain most of the functionality to do with payments and invoices (and more), so I’d expect their other APIs to be much smaller.

[judgej]

The bankfeeds API is an order of magnitude smaller; about tenth the size.

[judgej]

The ObjectSerializer::deserialize() needs some work.

It treats a string as JSON to decode and build into objects. It treats \SplFileObject as a streamed file, and attempts to check the headers to find a file name, which it then returns as a new \SplFileObject stream. I'm not sure why it writes the stream to a temp file, and does not use memory stream instead, as this has the danger of leaving temporary files around. Using memory is fine - PHP will automatically write to disk as soon as the memory stream reaches a small cache size anyway, so there is no limit on the stream size. But it will be faster and tidier.

The main point though is that a PSR-18 client will always return a Psr\Http\Message\StreamInterface stream regardless of the content type. You need to look at the content type header to find out what it represents. I suspect the Guzzle client did some additional checks on the content type to filter the streams from the JSON, and hinted what was being returned (or sent) by supplying either a body string, or a file stream. We need to do those same checks to determine what is being sent.

Looking at the file stream handling, if the response has a content disposition filename, then the intermediate file is created in the configured temporary area with the same name. If you have multiple requests being performed at once, to files of the same name, then these requests will start overwriting each other. I have not tested that, but this is just from observation. However, the deserialiser is not actually given any of the headers, so the content disposition is nor checked. I think this needs to go to a new issue to refactor deserialisation.

[judgej]

Some traits would be nice for functions that can be shared between multiple API classes, such as helper functions. Can additional templates and generated files be added to the template list without changing the Java generation logic? I've read that it can't, but it's worth exploring. Putting that code into partial templates should work though, and would be a step in that direction.

[bradydan]

@wing328 This repo is now public.

Please can you take a look at Jason’s comments above?