steamclock / netable

A Swift library for encapsulating network APIs using Codable in a type-oriented way.
MIT License
99 stars 3 forks source link

add smart unwrapping properties #88

Closed brendanlensink closed 3 years ago

brendanlensink commented 3 years ago

@nigel it feels like there's something here, but I'm starting to feel very stuck and not sure if I'm completely lost or just missing something small.

Inspired by this post, I've got something started with the idea that if you name your struct you're trying to unwrap the same as the field in the JSON, we might be able to use that to skip around the fact that we can't know the coding keys ahead of time?

I left some more comments in the code pointing out where I'm stuck, but also is this just too janky to be worth doing? Maybe this

nbrooke commented 3 years ago

I do think using the class name IS problaby a little too hacky, but I don't see why this technique would be limited to that, we can get the name from somewhere else if we need to (another parameter of the Request maybe?).

I think main conceptual issue you might be having is that this problaby also needs to involve the finalization stage. You can't change decode to return the actual unwrapped type because it HAS to return the RawResource type by the definition of Request, and the RawResource is SmartUnwrap. I THINK what we probably want in this case is for RawResource to be SmartUnwrap<Json> and the return type of decode to stay the same as it is, and then make FinalResource be Json and add a appropriately parameterized default implementation of finalize (something like where RawResource = SmartUnwrap, RawResource.Value = FinalResource, though I doubt that's actually the correct syntax for that) that pulls out the enclosed real object and returns it.

brendanlensink commented 3 years ago

@nbrooke updated, it works now! 🚀

It still feels a little janky to me, but I'm not quite sure if there's actually much more to be done.

Also, I decided to leave the default smartUnwrapKey as the name of the type, but added an option to overwrite it, since I thought it was kind of a neat way to save having to remember to declare it. I'm not super attached to it though, and if you feel like it's still too unintuitive/janky I'm happy to remove it.

nbrooke commented 3 years ago

I think there are still a few other things we can try to clean this up.

One question is: is it actually possible to just do this with Codable in some way, rather than having to round trip the data through JSONSerialization? It seems like it should be at least possible, either by having it try and decode to a dictionary (decoder.decode([String: FinalResource].self, from: data)) or by having some other wrapper type (maybe SmartUnwrap itself) actually be the thing decoded and have it have some sort of custom Codable implementation that can use dynamic key names, ala: https://swiftsenpai.com/swift/decode-dynamic-keys-json/.

Another thing to think about with the key name thing, rather then the default being "just the type name", what if the default were "assume there is only one key in the dictionary, and return that"? I don't think that could work with the dynamic keys approach mentioned above, but either the current implementation or decoding to a Codable dictionary could, I think, so basically the key name defaults to nil though you can set it if you want, and the matrix of results would be:

brendanlensink commented 3 years ago

Took another pass at this using roughly the strategy from that swiftsenpai link, I think this is a pretty nifty and clean approach.

I'm a little leery of just using the first object that decodes successfully in the SmartUnwrap.init, but I can't actually come up with a situation where it might cause a problem?

nbrooke commented 3 years ago

This is starting to look pretty good. It seems like smartUnwrapKey is still in there, but is not actually being used any more. If we aren't gonna use it we should definitely remove it (and should problaby remove the one that reads the name of the object no matter what). However I think we can / should still have that capability and that it provides a good solution to the "is unwrapping the first object a little too hacky" via the approach I mentioned above of only unwrapping the first object if it's not set .

To expand on that a little:

I think that provides a good mix of convenience (don't need to specify the key if there IS just a single field all the time), but in a case where there are mutiple key and ambiguity, you can use the smart unwrap key to resolve it.

brendanlensink commented 3 years ago

@nbrooke That sounds good to me, except for

If the smart unwrap key is not set and there are multiple keys in the object, that's an error.

I think there's a good chance that there's often going to be multiple keys, but only one actually decodes to the type you care about. Say for example, you've got a JSON object that returns with the thing you care about and some page or meta information, something like:

{
   "user": {
      "foo": []
   },
  "meta": {
     "bar": //...
  }
}

Do you think it makes sense to throw an error in this care? Or just return the object decoded if only one of the cases decodes properly?

brendanlensink commented 3 years ago

Updated with the changes suggested above, now when unwrapping it will:

  1. Check if there's a smart unwrap key, and if so only decode the object under that key
  2. If there isn't a smart unwrap key attempt to decode each key in the object a. If no objects get decoded, throw an error b. If >1 objects get decoded, throw an error c. If exactly 1 object was decoded, return that

I can see where you're coming from with returning an error if there's any ambiguity without setting a smartUnwrapKey, but I really don't like the possibility that a very common use-case results in an error (create smart unwrap without specifying a key), especially when the API could change and start returning a second completely unrelated object.

nbrooke commented 3 years ago

Yeah, I think that's a reasonable trade off. Looks to me like this is ready to go!