Remove SingleResource<T>

[GGAlanSmithee]

Remove the SingleResource<T> class

Today, we have two response types, SingleResource<T> and ListedResourceResponse<T>. The listed one is needed since it contains metadata such as offset and pagesize, but SingleResource is superfluous and probably not needed. Let's see if we can remove it.

CC @brokenprogrammer

[brokenprogrammer]

I completely agree, the following codesnippet:

var apiRequest =
    new FortnoxApiClientRequest<SingleResource<Invoice>>(HttpMethod.Put, request.AccessToken, request.ClientSecret,
        $"{ApiEndpoints.Invoices}/{invoice.DocumentNumber}")
    {
        Data = new SingleResource<Invoice> { Data = invoice}
    };

Doesn't really make sense, first of all why would we write new FortnoxApiClientRequest<SingleResource<Invoice>> when new FortnoxApiClientRequest<Invoice> really is the same thing..

I took a look at just not specifying SingleResource<T> but then we get some serialization problems in our requests when converting the object into JSON. If you follow the serialization problem it leads to CustomJsonConverter where it gives the Data property of the SingleResource<T> the specified name from the [JsonPropertyClass("")] attribute when serialized.

CustomJsonConverter WriteJson with SingleResource<Invoice>:

{
  "Invoice": {
    "AdministrationFee": 0,
    "Comments": "Comment: 2020-08-21 10:04:02",
    "CustomerNumber": "2",
    "EUQuarterlyReport": false,
    "Freight": 0,
    "InvoiceRows": [
      {
        "AccountNumber": 0,
        "ArticleNumber": "100370",
        "DeliveredQuantity": "10",
        "Discount": 0,
        "HouseWork": false,
        "Price": 0,
        "VAT": 0
      }
    ],
    "NotCompleted": false,
    "VATIncluded": false
  }
}

CustomJsonConverter WriteJson with Invoice:

{
  "AdministrationFee": 0,
  "Comments": "Comment: 2020-08-21 10:12:55",
  "CustomerNumber": "2",
  "EUQuarterlyReport": false,
  "Freight": 0,
  "InvoiceRows": [
    {
      "AccountNumber": 0,
      "ArticleNumber": "100370",
      "DeliveredQuantity": "10",
      "Discount": 0,
      "HouseWork": false,
      "Price": 0,
      "VAT": 0
    }
  ],
  "NotCompleted": false,
  "VATIncluded": false
}

[GGAlanSmithee]

Awesome. So maybe it makes sense to always lift the JSON one level? Is it safe to assume that is always the case?

[BaconFinger]

It is not always the case that the response looks like this. Invoice File Connections look like normal responses, and I'd postulate that any interaction with the "API Version" of their API (these are URLs that use .../api/..., instead of .../3/... for Version 3) have a more normal JSON structure that doesn't need any of this custom converter tomfoolery. I'm guessing this is because Version 3 supports XML and the JSON structure is just a JSON-ified XML.

After spending time with this I'm not as certain that we need to remove the SingleResource, or at least that we need to remove it in favor of messing around with a custom JSON converter.

It is strictly the case that any model we make will be wrapped by something when sending / receiving. I don't think it's particularly egregious if we're always returning the Data property in the services. For consumers, it's as if we're always working with the model itself. Perhaps it could suffice if we just rename SingleResource to something a little more clear (cough cough request/response)?

Using a custom JSON converter I think can lead to a whole bevy of questions and issues that are difficult to debug. Do we modify the existing converter or use a new one? Using a new one would be safest, but we'd have to make sure to remember to use it. Do we refactor everything to this new standard immediately? This is tricky due to the combination of a lot of tests and strict rate limiting.

Maybe instead of being fancy we just do something simple like:

public class Article {
  [JsonProperty(PropertyName = "Article")]
  public ArticleContent Content
}

public class ArticleContent : ArticleSubset {
// stuff
}

[GGAlanSmithee]

@BaconFinger

Good points, we shouldn't do anything just to do it. My grief with the SingleResource paradigm is that it doesn't add anything (other than maybe being consistent with the ListedResource) and it makes it such that the data structure has as nested data prop which is kind of ambiguous and unneeded. It isn't the biggest thing in the world though.

Regarding the custom JSON converter; we already do that though, right? Or do you mean introducing another one would open up for the possibility of using the wrong one?

[BaconFinger]

We already do that, but to drop SingleResource we'd need to either modify the existing one or create a new one. Modifying the existing one would require us to refactor everything that is already using SingleResource which would be hellish. Creating a new one would require us to use that one and slowly migrate everything over to using that one. It also edges close enough to witchcraft that I feel we'd be kicking ourselves for using it.

I think that the differences between version 3 and version API means that, conceptually speaking, SingleResource isn't that bad of a way to go.

[brokenprogrammer]

In my oppinion having a class like SingleResource that is tied into how JSON is parsed within the library is more error prone than rewriting the current JsonConverter to work without it and in the long run we'd benefit more from removing such parts from the library. I don't think it would be that hellish to refactor it, it basically just means to remove SingleResource from the services and make the json converter work without it.

Correct me if im wrong.

[GGAlanSmithee]

The burden would also be on the consumers of this SDK, since this is a change to the public-facing API and so, in accordance with semver, would require a major-version bump (2.0.0). Guess that means we need strong motivation to do this, but also means that we don't have to decide on it atm, since v2 isn't on the horizon just yet.