Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatic deserialize JSON to IContent implementations #12

Merged
merged 6 commits into from
Nov 25, 2020

Conversation

rasmusjp
Copy link
Member

This is still a WIP

Added automatic deserialization into custom models as described in #11 (comment)

Right now it works for Document Types but needs to be implemented for Element and Media Types.

@sitereactor
Copy link
Contributor

Changing the return type from Content to IContent would force you to cast it to something else as you would not have access the Properties property, which is not exposed on IContent.

@sitereactor
Copy link
Contributor

I'm wondering if it is necessary to change the return type. For the strongly typed models don't we inherit from Content anyway?
(Just thinking about a possible middle ground ... An alternative could also be to simple provide another set of methods that can return a generic list where the types are "hidden". But then again it might be confusing why you would use one over the other).

@rasmusjp
Copy link
Member Author

The reason for changing it to IContent was that I didn't want to inherit from Content, the reason is that you'll get the Properties dictionary but it'll only contain the "extra" properties that aren't present on the model, which I think might be confusing too

@sitereactor
Copy link
Contributor

Okay, but this also assumes that you are using strongly typed models right? Otherwise it wouldn't really be usable or at a minimum it would force you to cast it to Content.
And its the forced Cast and assumption I'm not to keen on.

Maybe an alternative could be to have GetRoot, GetAnscestors, etc. take a list of types in a method overload, so you essentially tell that you want the root items of types x and y in the returned list of IContent. Then at least its explicit.

@sitereactor
Copy link
Contributor

sitereactor commented Dec 11, 2019

Something like:

Task<IEnumerable<Content>> GetAncestors(Guid id, string culture = null);
Task<IEnumerable<IContent>> GetAncestors(Guid id, string culture = null, params Type[] modelTypes);

I realize there might be some overload clash here, but just an example. Method name could also be something like GetAncestorsTyped, GetAncestorsAsTypes.

@rasmusjp rasmusjp force-pushed the feature/automatic-deserialize-models branch from 5837b3d to 9870153 Compare January 2, 2020 14:51
@rasmusjp
Copy link
Member Author

rasmusjp commented Jan 2, 2020

Instead of creating overloads I've gone with the requirement of inheriting from Content, I think that's an okay compromise for now. Will you give it a look before I continue with Media?

{
public interface IHeadlessConfiguration
{
string ProjectAlias { get; }
ITypeList<IContent> ContentModelTypes { get; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a breaking change, so maybe consider adding another interface to add the ITypeList<IContent> ContentModelTypes and leave IHeadlessConfiguration untouched.
For the implementation and the public class we could then choose to only expose the one you implemented in HeadlessConfiguration.

We'd of course need to check on ContentModelTypes, so we can pass an empty list, but think that would be okay.

Copy link
Contributor

@sitereactor sitereactor Jan 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, and since ContentDelivery is internal we should be able to change the constructor so we can change the interface that is passed in.

@sitereactor
Copy link
Contributor

sitereactor commented Jan 2, 2020

I think this will work fine. Having to inherit from Content is an okay alternative. Of course you get the Properties property as part of the object, but it gives a bit more flexibility in terms of types for those that want it.
I'm unsure though, if the Properties property on Content will still be populated with values when using a strongly typed model with a different set of properties

One thing that we will need to ensure before this is released is that it still works with Xamarin (UWP, Xamarin.Android, Xamarin.iOS). There are some limitations around reflection, so just want to ensure that we don't inadvertently break compatibility.

@rasmusjp
Copy link
Member Author

rasmusjp commented Jan 6, 2020

Properties will only be populated with values that cannot mapped, so if a property exist on the model it's value won't be available in Properties.

Regarding UWP and Xamarin we should be good, as far as I can see it's Relection.Emit and Reflection over private members that isn't allowed (see https://docs.microsoft.com/en-us/dotnet/framework/net-native/reflection-and-net-native and https://docs.microsoft.com/en-us/xamarin/ios/internals/limitations)

@rasmusjp rasmusjp marked this pull request as ready for review January 6, 2020 14:38
@rasmusjp rasmusjp force-pushed the feature/automatic-deserialize-models branch from 1a96e04 to 84db6cc Compare January 6, 2020 14:45
@rasmusjp
Copy link
Member Author

rasmusjp commented Jan 6, 2020

This is ready for review now.

I've added a new interface IStronglyTypedHeadlessConfiguration (not sure about the name) which contains 3 properties ElementModelTypes, ContentModelTypes and MediaModelTypes where the strongly typed models can be registered. An example can be found in the .NET Core MVC sample, Image, File and Folder are registered by default for media if you use any of the default configuration classes.

The (content/media) type alias that is used to lookup the model types based on the class name, this can be overwritten by adding one of the following attributes to the model, ElementModelAttribute, ContentModelAttribute or MediaModelAttribute

@NiclasLindqvist
Copy link
Contributor

This looks great, seems to be working as intended from what I've seen so far! 👍 Is there any possibility to allow "casting" to PagedContent<T>. e.g. when using GetChildren (where using GetChildren<T> is possible) or when using Filter (where no Filter<T> is available)

If it was possible to call something like the following, that'd be very helpful!

var filters = new List<ContentFilterProperties>() {
    new ContentFilterProperties("property", "value", ContentFilterMatch.Contains))
};
var contentFilter = new ContentFilter("thing", filters.ToArray());
var items = (PagedContent<Thing>) await _contentDelivery.Content.Filter(contentFilter);

@NiclasLindqvist
Copy link
Contributor

The above mentioned was somewhat resolved by #19 as you can now do both GetChildren<T> and Filter<T>. Adding support for Search<T> would possibly be useful as well, however it is hard to guarantee the result from such a rudimentary search is only of the requested type I suppose.

@rasmusjp rasmusjp force-pushed the feature/automatic-deserialize-models branch from 84db6cc to ba0f652 Compare November 23, 2020 14:45
@rasmusjp
Copy link
Member Author

I've rebased and updated the code.

I've opened a new PR with a Xamarin Forms demo #31 we can use to test that the library is still working with Xamarin

@sitereactor sitereactor merged commit 0842180 into master Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants