Code review comments

[chihoyeung]

Here are my comments after doing a code review.

Canonization
AttributeCanonizationRule.cs
Canonization.cs

• I wish I had means to canonize a list of Multi-attributes to single item list, e.g. application supports only single email.
• I wish I could canonize $ref -> value/type and vice versa, but that only applies (but it can be implemented similar to EnforceSinglePrimaryAttribute, just not be stateful)
• return in EnforceScimUri duplicated
  Configuration
  ResourceParameterBinding.cs
• there is a TODO in RequiredResourceExtensionCache, really needed?
  ScimApplicationManager.cs
• I don't like what I added in line 265. I no longer have the need to override OwinScimExceptionHandler. The only reason I did at the beginning was to introduce logging, but the proper way was to add a IExceptionLogger service which I did.
  ScimServerConfiguration.cs
• CreateDefaultFeatures sets bulk to supported.
  Dependencies
  FuncDependencyResolver.cs
• return null in catch. Though harmless because it is going to fail Owin.Scim as "could not resolve this type", could make it harder to debug because it will hide the original exception.
  Endpoints
  ScimControllerBase.cs
• GetGroupUri and GetUserUri (I wrote these two) can be combined into one.
  Extensions
  AppBuilderExtension.cs
• what is the purpose of the UseScimServer() without fileCompositionConstraints parameter? When I call it adds Owin.Scim.dll as Predicate twice.
• why does fileCompositionConstraints affects what objects can be injected from the provided DependencyResolver?
  Users
  ScimUser.cs
• I wonder if it would be a good idea to default Active to true. For the case where I post, and don't send the Active parameter.
• should we use Password to calculate version? that will force implementers to provide decrypted password.
• I'll rather not use Groups to calculate version, since this are derived values.
  Querying
  ScimFilterVisitor.cs
• various exceptions need better description
  Services
  GroupServices.cs
• I don't need to set group.Meta in CreateGroup, it's already set in constructor, and it should not be deserialized.
• I wish to canonize group before returning the value.
• in UpdateGroup, setting group.Meta.LastModified from current record is wrong.
• should move where I pick the first validation error, down to where I generate ScimError
  UserService.cs
• same thing with Meta
  Validation
  ResourceExtensionValidatorBase.cs
• ExistingRecord property not used anywhere, nor "resource" logical data ever set.

Endpoints
GroupsController.cs

• I need a way to override Patch so when I retrieve a group, it lists all members regardless of visibility rights.
  AsyncLazy.cs
• not used anywhere.
  v1.Endpoints
  GroupsController.cs
• Do not like that we are setting Members.$ref here in the controller, maybe move it to service? what about doing the same for SetMetaLocation?
  v1.Model
  ScimUser1Definition.cs
• I'll like to see a cannonization rule for Groups where value/type is used to populate $ref, and vice-versa
• how do we set the reference type for the $ref in multi-attributes like Photos?
  v1.ScimConstantsV1.cs
• Messages.Error and Messages.SearchRequest set to 2.0

v2.Endpoints
GroupsController.cs

• I'm having problem customizing the Patch, namely retrieving group with a fully populated member list that is not filtered based on rights. Should some of the logic move to the service?
• TODO in Patch, what is left to finish?