Proposal: support the Portable Class Library
Closed this issue · 26 comments
I made an attempt tonight to convert this project to PCL. The attempt failed, but wasn't too far off on the GEDCOM projects. The test projects and command line items do not need to be ported to PCL, so I first converted all the GEDCOM X * projects to profile259. Json.Net has support for 259. That went fine. I had to remove all of the SOAP attributes after that. I think we could safely ditch SOAP for a PCL library. JSON has totally conquered the SOAP world anyhow. I changed usages of Stack to Stack<object>. (BTW, code exposing that publically doesn't appear quite right, either.) typeof(T).IsAssignableFrom(extension.GetType()) should obviously be extension is T.
After those changes, I was left with some miscellaneous items that were more difficult. ZipArchive is going to have to change to something from Microsoft.Bcl.Compression. The TimeZone call is going to have to change to some 3rd party solution. RecordHelper is going to have to ditch DataTable. And the usage of IsAssignableFrom in the JsonConverters; we need a different plan there. These items would all take some work.
My point is: does anyone else want this feature? Is it worth working on? Wait for the next version of .NET?
I had attempted this back in Oct '14 (before the major rewrite) and was also unsuccessful. I am currently using a fork of this repository in a Xamarin.Forms project that compiles to iOS and Android. So, if you do end up converting to PCLs, I would prefer that it supports Xamarin.Android and Xamarin.iOS if at all possible.
Here are some links with information:
http://developer.xamarin.com/guides/cross-platform/application_fundamentals/pcl/introduction_to_portable_class_libraries/
http://danrigby.com/2014/04/16/xamarin-pcl-profile-notes/
So you're proposing that we target profile 111?
@BrannonKing When you say "SOAP attributes" are you referring to the SerializableAttribute decorations throughout the various classes? If so, the main reason is for reading and writing GEDX files from disk (see XML File Format Specification). Furthermore, XML could be specified for web API calls (although JSON is the default). I'm confident the approach is arbitrary, but a PCL will require XML serialization support.
Each of the objects presently has a SerializableAttribute on it. That attribute is meaningless without using the BinaryFormatter or the SOAP serializer. (See here: http://stackoverflow.com/questions/2982376/why-is-serializable-attribute-required-for-an-object-to-be-serialized). It is no longer supported and has to go away for PCL.
There are three other attributes: Xml_, Soap_, and Json. The newer XML serializers and the JSON serializers all support DataContract/DataMember. It seems that those should be the attributes of choice as they are fully supported in PCL as well. However, I think we can get rid of the Soap*Attributes and still fully support XML serialization with the older attributes that are there. Who is using SOAP and what for?
I'm confident that as long as the ability to read and write GEDX files remains, no one will care how it's accomplished. I think I wrote a number of functional tests that can validate any core changes like this.
The Soap*Attributes were part of the project before I ported the Java SDK. I'm not sure why they were there in the first place, but I simply left them for the sake of not breaking anything while porting.
@stoicflame - Can you address any need for Soap*Attributes?
Yeah, they're artifacts of an older age. I'd have no problem removing the soap attributes.
Soap attributes removed at #29. Thanks @shanewalters.
The code below is not supported in profile 111. Somebody give me an equivalent or tell me what it is doing. There is no XmlAttribute type available. It is from Entry.cs.
[System.Xml.Serialization.XmlAnyAttributeAttribute()]
public System.Xml.XmlAttribute[] OtherAttributes
{
get
{
return this._otherAttributes;
}
set
{
this._otherAttributes = value;
}
}Somebody give me an equivalent or tell me what it is doing.
It's a "catch-all" bag for attributes that the model doesn't "know about." The Atom Entry element is defined to take some "known" attributes, but it also says that you can really put any attribute there, if you want. So this property is the catch-all. According to the docs for XmlAnyAttributeAttribute the property must return an array of XmlAttribute objects.
I don't have any suggestions for an equivalent. The only workaround I can think of is to remove that property and remove support for the "any attribute" from this serializer.
The StateFactory.LoadDefaultClient method uses an environment variable to add a Log4Net logger. Log4Net doesn't support PCL and PCL does not support environment variables. It seems that I could change this to use System.Diagnostics.Tracing. Does that sound sufficient?
Replacing Log4Net with Tracing will be fine. I chose Log4Net while porting due to its logging flexibility, but it's definitely not a requirement.
I worked on this for several hours in earnest today. I converted all of the GedcomX projects and the Link project. Those went fairly quickly once I learned about GetTypeInfo. The real work began when I got to the Rs.Api project. I hadn't realized that the RestSharp project was not portable. From what I'm reading there are four options: Restsharp.Portable, PortableRest, Flurl.Http, and direct use of HttpClient (which I'm sure wasn't available when the project was originally written). Restsharp.Portable has very similar interfaces to Restsharp. I'm thinking that is the best replacement option. I went down that road for some time. It's doable with some more work. Any thoughts on this?
So long as the 225 functional tests pass*, the underlying library communicating with the backend FS API is arbitrary. I settled on RestSharp primarily out of being the most feature complete compared with our needs.
*One important feature to support (which I don't know if it's covered by the functional tests) is the need to communicate with the FS API in either XML or JSON. This SDK defaults to JSON, but could use XML.
Well, folks, I've got the project converted in my branch. See here:
https://github.com/BrannonKing/gedcomx-csharp
I have nine failing tests. I could use a little help with those -- anybody willing to take a look? It appears to have something to do with how we interpret "no content".
Breaking changes:
- I moved the method to convert RecordSet to DataTable into the command line project.
- I broke the serialization on GedcomxApplicationException. I have no idea where that was used.
- The content-type-to-deserializer is now explicit per content type.
Really, I made a mess in the FilterableRestClient class. That's where most of the changes are. I haven't yet thought through how I would use injection to configure that thing. The library is somewhat injection adverse; it must have arisen before the SOLID principles really caught hold. Lots of constructors do a lot more than they should, including connect to the server. The portable rest client threw an exception for 401 by default. Fortunately I was able to disable that feature as it broke everything requiring authentication. Apparently we connect when the objects are constructed and let that fail silently before specifying the credentials; it's an unnecessary server hit.
Wow. Nice work.
I moved the method to convert RecordSet to DataTable into the command line project.
That seems fine to me.
I broke the serialization on GedcomxApplicationException. I have no idea where that was used.
It wasn't used. It was an outdated artifact.
The content-type-to-deserializer is now explicit per content type.
Okay, I'm not totally familiar with this mechanism, but it sounds reasonable.
Really, I made a mess in the FilterableRestClient class. That's where most of the changes are.
Well, I'm okay with that as long as the functional tests pass. Have you been able to verify that everything is passing?
Like I said, not all the tests pass. 9 still fail with "NoContent"-type of errors. Mind taking a look?
Okay, I realize the last comment on this thread was 6 months ago, but I finally got around to looking at the failing tests. Following are my observations on each failing test.
I'd love to be able to merge this fork; I think supporting the PCL would be a good enhancement. Let me know when you get the tests passing and I'll merge it.
TestDeletePersonWithPreconditions
The attempt to delete the person is supposed to fail because it's providing preconditions; the client says "only delete this person if it has not been modified since this date." For some reason it's not failing (ifSuccessful doesn't throw an error). This could be because the If-Unmodified-Since header isn't getting sent, or perhaps because the update on line 445 isn't actually updating something.
TestDeletePreferredParentRelationship, TestDeletePreferredSpouseRelationship
The failure's pretty straightforward to explain. It expects there to be a Content-Location header in the response, but it's not there.
TestInitiateAuthorizationInvalidParameter
I think this is an invalid test. It's expecting some HTML element that's not there. I think we shouldn't write the test based on the existence of a particular HTML element. I suggest removing the test, or just not doing the assert on line 113.
TestReadCurrentTreePersonExpecting200Response
There's no reason why this test should fail and the TestReadCurrentTreePerson should succeed. There's got to be something that the underlying request doesn't like about using the Expect header or something.
TestReadPersonPortraits
It seems like this test is expecting a portrait to be set for the person, but it's not. So instead of a 200 response code, it's a 204 (No Content). I'd suggest just asserting a "No Content" response instead of asserting the 200.
TestReadPlaceTypeGroup
I think this test might be invalid. The place type group of id "1" is invalid. I'd suggest just removing the test.
TestRestoreChildAndParentsRelationship, TestRestoreCoupleRelationship, TestRestorePerson
These tests are all failing with a NullReferenceException. That should be pretty easy to debug.
Do either of you know what type of PCL you are wanting to target? I see a note about Xamarin Forms. Is there interest in this feature?
There is a total interest in this feature. I strongly vote to address this issue and make FamilySearch accessible for UWP and Xamarin.
Looks like the only issue here is the log4net library. Real shame!
This is totally doable through the C# lite SDK. You no longer have the big code base to deal with. Check out https://github.com/FamilySearch/csharp-lite
Oh wow I didn't know about the C# lite project.
Is there a comparison on the feature list of these two projects full vs. lite?
The C# lite project shall replace the original C# Family Search SDK, however several parts of that SDK can still be utilized. The difference is helping with OAUTH2 connections, optionally using the models of the old SDK and being a light footprint in your code base. The maintenance of the this full SDK has been a challenge and so the new C# lite SDK was created.
I read this on the docs there and read your text in there, but since the lite project is not on nuget, and since I haven't seen anywhere mentioning the FS API (only GedcomX), I was wondering whether it also targets all the features of the FS API.
Do you have experience with it?
Please take a look at the readme on https://github.com/FamilySearch/csharp-lite . It is on Nuget and at the end of the readme is a sample application I wrote. Feel free to contact me directly about the C# lite project for further questions as this is a thread on PCL support.
The Gedcomx.Lite packages don't support portable apps. I Just tried.
Anyway, would you say the lite version is anyway the version is going take over this entire gedcomx-csharp repo?
Any updates?
I can't install Gedcomx.API.SDK to a UWP app.