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

.Net Core compatibility #333

Merged
merged 17 commits into from May 23, 2017

Conversation

Projects
None yet
2 participants
@CBaptiste
Member

CBaptiste commented Mar 27, 2017

No description provided.

@CBaptiste

This comment has been minimized.

Member

CBaptiste commented May 3, 2017

@ChrisMcKee back on this pr (I forked yours)
After a few testing, I think we are loosing the .Net 4.0 compatibility no ? or I am mistaken ?
I am testing the compatibility of the PR with a few different platform

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented May 3, 2017

@CBaptiste I set the base level at 4.5
https://github.com/ChrisMcKee/algoliasearch-client-csharp/blob/63a1090d99cf51eb6856e26218750d8c7698bcbc/Algolia.Search/Algolia.Search.csproj#L20

Technically anything prior to 4.6.2 is out of support anyway https://blogs.msdn.microsoft.com/dotnet/2015/12/09/support-ending-for-the-net-framework-4-4-5-and-4-5-1/ but I habitually keep lower targets.

As in place replacements, no one should be using less; I'd be surprised if someone was using algolia and running a 6 yearold stack

ChrisMcKee and others added some commits Mar 15, 2017

Resolves #259; Add support for .net core / standard. Remove reliance …
…on PCL. Drop depreciated targets...

* Set target frameworks in csproj; remove depreciated platforms https://docs.microsoft.com/en-us/nuget/schema/target-frameworks
* Set nuget defaults based on current algolia csharp on nuget site; you can see the properties here https://docs.microsoft.com/en-us/nuget/schema/msbuild-targets
* Drop PCLCrypto , at a wild guess this was 5 years ago to support winrt; as it stands it could have been handled in a single helper class using compiler tags.
 * https://msdn.microsoft.com/en-us/library/system.security.cryptography.hmacsha256(v=vs.110).aspx
* Remove TaskEx (not been a valid class since 2012 CTP)
 * https://msdn.microsoft.com/en-us/library/system.threading.tasks.task(v=vs.110).aspx
* Correct framework in tests; generally use your own assembly namespace, not NUnits
* Swap auth key / api key test values for ones from python, lifes easier when all the clients play together. (TestGenerateSecuredApiKey)
* Swap "àlgol?à-csharp for plain text; if this is supposed to test UTF-8 or similar then use test cases and a specific test to create/destroy indexes
Change naming to match c# conventions (all a tad mixed) / correct typ…
…os and missing xmldoc / correct enumeration

* _dsnInternalTimeout is styled as if private; when public and publically set in the tests
* CT Token taken in but not passed along in WaitTaskAsync, time to wait also missing in xml doc (necessary as its int not timespan it could be anything)
* use base types where possible (string not String)
* in DeleteObjectsAsync enumerable taken in is string, foreach type should be string not object.
* access before async (public async not the more javsacript looking async public)
* weirdly manual test for null or empty on .net string type,  `cursor != null && cursor.Length > 0` gains no optimization.
* use of ienumerable in SearchDisjunctiveFacetingAsync, foreach will enumerate, contains will enumerate again; converted to array though list would of been fine.

CBaptiste added some commits May 12, 2017

@CBaptiste CBaptiste force-pushed the core branch 8 times, most recently from 988ae1f to 51ddfce May 12, 2017

CBaptiste added some commits May 12, 2017

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented May 15, 2017

I changed it to match the identical input => output of the python driver (needed a sane source to work against) as the outputs in .net don't match. All the integration tests passed at the time so I did my happy dance and pushed.

@CBaptiste

This comment has been minimized.

Member

CBaptiste commented May 16, 2017

ok so @ChrisMcKee I quickly tested the secured api key we are generating with your change, the engine says it is invalid, I m looking into it

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented May 16, 2017

image

You'd see why I'd be confused by this 😛
Granted this is on my branch; I've not pulled any changes in.


Ok so the tests dont use the secured api key*

@CBaptiste

This comment has been minimized.

Member

CBaptiste commented May 16, 2017

maybe there is a misunderstood somewhere :)
your PR: https://github.com/algolia/algoliasearch-client-csharp/pull/331/files#diff-3d44626428ff506b9c026a18d6f1ea00L895
the test on the API keys changed, and those new API keys are invalid

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented May 16, 2017

@CBaptiste Yeah it didn't click; its was March, lots of codes gone by since then 😉
The hashes I generated for the left side came from the standard python hashlib

import hashlib
import hmac
import base64
from pprint import pprint

securedKey = hmac.new("182634d8894831d5dbce3b3185c50881".encode('utf-8'), "tagFilters=%28public%2Cuser1%29&userToken=42".encode('utf-8'), hashlib.sha256).hexdigest()
pprint (base64.b64encode(("%s%s" % (securedKey, "tagFilters=%28public%2Cuser1%29&userToken=42")).encode('utf-8')).decode('utf-8'))

(based off https://github.com/algolia/algoliasearch-client-python/blob/6b8832fbc1d180ae41813c04bda2766757156822/algoliasearch/client.py#L511-L535 )

One I just generated for my own came back

ZTBlZWNmZWEyNmIxMWFiNWRhODBhMzdhNTBiOGM4YzRmNTYwMzU5NjdkZGQ5ZmFlNTk0Nzc5NzU3ZWM2NGIwMHJlc3RyaWN0SW5kaWNlcz1hbGdvbGlhLWNzaGFycA==   C#

ZTBlZWNmZWEyNmIxMWFiNWRhODBhMzdhNTBiOGM4YzRmNTYwMzU5NjdkZGQ5ZmFlNTk0Nzc5NzU3ZWM2NGIwMHJlc3RyaWN0SW5kaWNlcz1hbGdvbGlhLWNzaGFycA==   Python

But yep I can replicate the issue by changing test initialise on AlgoliaClientTests to

        [SetUp]
        public void TestInitialize()
        {
            _testApiKey = Environment.GetEnvironmentVariable("ALGOLIA_API_KEY") ?? "mykey";
            _testApplicationID = Environment.GetEnvironmentVariable("ALGOLIA_APPLICATION_ID") ?? "TJ8JVLYYX7";

            _client = new AlgoliaClient(_testApplicationID, _testApiKey);

            _index = _client.InitIndex(safe_name("algolia-csharp"));
            //_index.ClearIndex();
            _index.AddObject(JObject.Parse(@"{""firstname"":""Jimmie"", ""lastname"":""Barninger""}"));

            string secureAPI = _client.GenerateSecuredApiKey(_testApiKey, new Query().SetRestrictIndices(new[] { "algolia-csharp" }));

            _client = new AlgoliaClient(_testApplicationID, secureAPI);
            _index = _client.InitIndex(safe_name("algolia-csharp"));
            _index.AddObject(JObject.Parse(@"{""firstname"":""Jimmie"", ""lastname"":""Barninger""}"));

        }
@CBaptiste

This comment has been minimized.

Member

CBaptiste commented May 16, 2017

interesting,
When building the query string, Uri.EscapeDataString in GetQueryString does not generate the same string in both project with the exact same parameter
in the old PCL: tagFilters=(public%2Cuser1)
in the new: tagFilters=%28public%2Cuser1%29

I don't know if it is linked to it at least that s a first behavior difference

@CBaptiste

This comment has been minimized.

Member

CBaptiste commented May 16, 2017

ok, prior to 4.5, values are not encoded

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented May 16, 2017

Just to be extra clear*
I first went through the other projects
image
To get that hash string; The algolia python lib does not directly manufacture the hmac and uses a library so I worked towards getting a match between the two.
And prior to 4.5 vals wouldnt have been encoded

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented May 16, 2017

Lol jinx 👍

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented May 16, 2017

@CBaptiste the HMAC itself is fine
image
After that its only converting to base64

e0eecfea26b11ab5da80a37a50b8c8c4f56035967ddd9fae594779757ec64b00restrictIndices=algolia-csharp

becomes

ZTBlZWNmZWEyNmIxMWFiNWRhODBhMzdhNTBiOGM4YzRmNTYwMzU5NjdkZGQ5ZmFlNTk0Nzc5NzU3ZWM2NGIwMHJlc3RyaWN0SW5kaWNlcz1hbGdvbGlhLWNzaGFycA==

in online "to base 64"

in .net its

ZTBlZWNmZWEyNmIxMWFiNWRhODBhMzdhNTBiOGM4YzRmNTYwMzU5NjdkZGQ5ZmFlNTk0Nzc5NzU3ZWM2NGIwMHJlc3RyaWN0SW5kaWNlcz1hbGdvbGlhLWNzaGFycA==

Colour me confused 😕

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented May 16, 2017

@CBaptiste hey can you test this using your python client....

import hashlib
import hmac
import base64
from pprint import pprint
from algoliasearch import algoliasearch


client = algoliasearch.Client("TJ8JVLYYX7", 'myKey')

public_key = client.generate_secured_api_key('myKey', {'restrictIndices':'algolia-csharp'})

print(public_key)

clientb = algoliasearch.Client("TJ8JVLYYX7", public_key)

index = clientb.init_index("algolia-csharp")
index.add_objects([{"firstname":"Jimmie","lastname":"Barninger"}])

algoliasearch.helpers.AlgoliaException: Invalid Application-ID or API key

I get the same error regardless of client; wondering if theres an issue beyond the library here

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented May 16, 2017

"search only api key" helps if I read before I type 💃

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented May 16, 2017

Seems fine 👍

Python (working secure api key)
NTk2ZWZiN2NkYTk3OWFkOGViOGZmNWU5ZjZiOTAyMWUyODc5OWJjMzI3MTIzMjc3OWE2OTU0ZGI5YjcyNWVlM3Jlc3RyaWN0SW5kaWNlcz1hbGdvbGlhLWNzaGFycA==
C#
NTk2ZWZiN2NkYTk3OWFkOGViOGZmNWU5ZjZiOTAyMWUyODc5OWJjMzI3MTIzMjc3OWE2OTU0ZGI5YjcyNWVlM3Jlc3RyaWN0SW5kaWNlcz1hbGdvbGlhLWNzaGFycA==

Slight crossed wires I think; I couldnt get any of it to work as I was testing against an admin key to generate a secure api key. You were just fighting the one with brackets (I assume)

@CBaptiste

This comment has been minimized.

Member

CBaptiste commented May 16, 2017

The issue with the keys are fixed in that PR (c# client, I have the hash that passes th test with the original valid keys) but now I get some weird time out with 1 specific test, and then we are good...

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented May 16, 2017

@CBaptiste cool... give it a couple of months and netstandard 2 😉

CBaptiste added some commits May 17, 2017

@CBaptiste

This comment has been minimized.

Member

CBaptiste commented May 19, 2017

XUnit was running tests in parallel and was messing everything up.
Nugget about to be released \o/
We will be dropping 4 and 4.5 support

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented May 19, 2017

Woot! Awesome

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented May 22, 2017

@CBaptiste this going today/morrow?

@CBaptiste

This comment has been minimized.

Member

CBaptiste commented May 23, 2017

yes that s the plan !

@CBaptiste CBaptiste merged commit 8959a3d into master May 23, 2017

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented May 25, 2017

@CBaptiste

This comment has been minimized.

Member

CBaptiste commented May 25, 2017

I cannot thank you enough @ChrisMcKee ! \o/

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented May 26, 2017

@CBaptiste no probs; see you in a few weeks for round two 💃

@Ant-hem Ant-hem deleted the core branch Oct 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment