Create Access Key

[masojus]

[IshamMohamed]

I would like to participate in the Access Keys project

[masojus]

Hi @IshamMohamed, I could help with that. Have you started prototyping or thinking about a design at a high level? I'd love to hear what ideas you have.

[masojus]

Also, if you're looking for inspiration, you could look at the recent Python PR that added Access Keys and the ongoing PHP PR doing the same.

[IshamMohamed]

The Python create_access_key returns the JSON, so is it enough if we return the JSON or do you think we need to created objects?

[IshamMohamed]

I have created the model class for the Access Keys - https://github.com/IshamMohamed/keen-sdk-net/blob/master/Keen/AccessKey/AccessKey.cs would appreciate your feedback.

[masojus]

Thanks @IshamMohamed I will take a look.

[masojus]

I think the model is starting to look good. A few questions/comments:

• I'm not sure why you need ProjectId on the AccessKey type. It needs to go in the URL but not in the POST body, AFAIK.
• When serializing, it'd be nice to exclude the Key property, probably via NullValueHandling.
• Options.Queries really only needs the filters member, and I doubt IQueries will get serialized/deserialized properly here. This should probably just be another class with a Filters property that is just an IEnumerable<> like in the SavedQueries model class.
• Writes.autofill should be Writes.Autofill, and if it's going to be dynamic we won't need the Autofill class unless you plan to do something with extensions/helpers/factory/builder for the autofill properties.
• I assume which properties of Options get serialized will be controlled by NullValueHandling also.
• At some point we'll want to add some helper code for initializing Access Keys that makes sure there aren't inconsistencies, but that doesn't necessarily need to be in the first iteration.
• I'm not sure using string[] is any better than IList<string> or IEnumerable<string> for some of those properties. Any reason you chose an array for some properties and IEnumerable<> for others?
• I don't believe Datasets.Allowed needs to be dynamic since it has a more rigid structure than Writes.Autofill, seeing as how it's just an IDictionary<string, IDictionary<string, IDictionary<string, IEnumerable<string>>>> but really we know there can only be an optional index_by and it can only have one thing, and we know what that looks like, so we could have an AllowedThingy model class enforcing that structure and Datasets.Allowed could be an IDictionary<string, AllowedThingy> (pick a better name, of course). Then AllowedThingy.IndexBy could also be an instance of an IndexBy class or just a dictionary too, whatever makes sense.
• Returning JSON might be OK for the first iteration. I'd prefer eventually to make things as dev-friendly as possible while staying flexible, but go ahead and flesh out a working version returning JSON and then we can wrap that in something nicer if that seems necessary and makes sense. That way at least this becomes functional as soon as possible. Or just return whatever model class instance is appropriate, if that works...like the AccessKey instance.
• Probably the biggest issue to figure out how to handle is use of the access key. Right now there's this notion of MasterKey, ReadKey and WriteKey, and they're all strings, so in theory you can plug an Access Key in there anywhere you want, but it seems a little sketchy. In the past when thinking about this, I've considered making a bigger change to require a key with every command or add an AccessKey to the settings type and fallback logic in the appropriate places to use that as a last resort if the other expected keys aren't there, but then what happens if you want to have a master key as fallback and an AccessKey there as an override if present (or vice versa), especially with how keys are currently copied when the impls are created (like the Queries class). Anyway, again this doesn't need to be solved at first, because I think it should work if you shove this key into the appropriate place in the settings object, so we can tackle it later.

[IshamMohamed]

Really useful comments and feedback. Agrees with all your comments. I am learning alot, and have started working on the changes.

[masojus]

Good to hear! Yeah if you have good reasons for any of your choices, please feel free to correct me. I've thought about this Access Keys stuff to some extent, but I'm glad someone is finally getting it done :)

Another question: Is there a reason for AccessKey.Permitted to be a List<> instead of some other collection type (e.g. set in this case)? List<> tends to imply ordering or wanting to be able to index into the collection, which is fine if that's what we want.

Also, FYI, you could see trouble where you're trying to deserialize to interface types, though, since without a specific JsonConverter, Json.net doesn't know what concrete type to deserialize to. (if the model is an interface, that is...maybe here it's fine since they're all concrete types.)

[masojus]

AccessKey.Permitted could really be an ISet<> since they should be unique. They'd still be an IEnumerable<> that way but would enforce uniqueness. Same thing for blocked and allowed since they really can't be duplicates and ordering doesn't matter.

[IshamMohamed]

Implemented the method and a unit test for testing success - https://github.com/IshamMohamed/keen-sdk-net/commit/3596593fe4eef37484e5e8d93a6eaa526f08d08b

[masojus]

Thanks @IshamMohamed I'll take a look probably later today.

[masojus]

Can you create a pull request when you're ready so we can review this here? I'd prefer to have the context.