[ODS-5665] FOR PRELIMINARY REVIEW #1156
Conversation
|
|
||
| public static EntityProperty FindIdentificationCodeProperty(Entity entity) | ||
| { | ||
| var entityFullName = entity.FullName.Name; |
There was a problem hiding this comment.
Probably just use entity.FullName here because it is a struct and includes the schema. Dropping the schema delineation could be problematic due to the possibility of a "homograph" (an extension with a resource of the same name).
| using EdFi.Ods.Common.Specifications; | ||
|
|
||
| namespace EdFi.Ods.Common.Providers.Criteria; | ||
|
|
||
| internal static class AggregateRootCriteriaProviderHelpers | ||
| { | ||
| private static string[] _uniqueIds; | ||
| private static readonly ConcurrentDictionary<string, EntityProperty> _entityIdentificationCodePropertyCache = []; |
There was a problem hiding this comment.
Rename this to _identificationCodePropertyByRootEntityName. This makes it clear what the dictionary is keyed by.
| public static EntityProperty FindIdentificationCodeProperty(Entity entity) | ||
| { | ||
| var entityFullName = entity.FullName.Name; | ||
| var identificationCodeEntityProperty = _entityIdentificationCodePropertyCache.GetValueOrDefault(entityFullName); |
There was a problem hiding this comment.
You should use the GetOrAdd mehtod of ConcurrentDictionary and go ahead and store the null value if no identification code is found to avoid doing this logic for a particular entity more than once. We definitely don't want to be repeatedly looking through the model for the same entity time and time again.
|
|
||
| EntityProperty FindIdentificationCodePropertyInAssociations(Entity entity) | ||
| { | ||
| return entity.OutgoingAssociations |
There was a problem hiding this comment.
You'll want to use NavigableChildren for this... only search within the resource/aggregate. OutgoingAssociations includes all outbound associations, even FKs to other resources.
| : "r"; | ||
|
|
||
| var identificationCodeTableJoin = CreateIdentificationCodeTableJoin(entityIdentificationCodeProperty, alias); | ||
| queryBuilder.Join(identificationCodeTableJoin.TableName, _ => identificationCodeTableJoin, JoinType.LeftOuterJoin); |
There was a problem hiding this comment.
I would think you would want to use an INNER JOIN here, and only activate this logic if there are actually parameters supplied that match a search for the identification code on this particular entity.
In other words, just because there's an identification code defined for the entity/resource being requested doesn't mean we want to introduce any joins to the identification code child table, and certainly not a left join which is more expensive. We only want to perform a join if they are providing identification code-related parameters and then the join should be an inner join.
So, in the other function, once you've identified there's an IdentificationCode property on a child table, you should go ahead and create a dictionary of entity properties keyed by the corresponding parameter names for that child entity. (After writing this comment, I see that you've basically got that function written already at the bottom of this class. It just needs to be refined a bit and reusable by the Open API metadata generation process.)
Also, remember to store this dictionary (or a null value if there's no identification codes to be found for it) in the top level ConcurrentDictionary in the applicator class (keyed by the Entity.FullName). This will allow you to quickly determine first if there is any identification codes for the entity without doing the work each time, and then second, if there are any matching parameters supplied in the incoming additionalParameter parameter (dictionary). Only if both of those conditons are met would you proceed to building the INNER JOIN to the identification code table.
| public void ApplyAdditionalParameters(QueryBuilder queryBuilder, Entity entity, AggregateRootWithCompositeKey specification, | ||
| IDictionary<string, string> additionalParameters) | ||
| { | ||
| if (additionalParameters == null || !additionalParameters.Any() || additionalParameters.All( |
There was a problem hiding this comment.
I think you could leave the null and the Any checks, but I don't think the latter is needed because ASP.NET will fill that dictionary with values that were not bindable to the other models in the controller signature. We should confirm this, but I think this will be wasted worked. Thoughts/concerns?
|
|
||
| return null; | ||
|
|
||
| EntityProperty FindIdentificationCodePropertyInAssociations(Entity entity) |
There was a problem hiding this comment.
This method name is misleading because you're looking for a property in the child table, not the association (aka FK). The identification code will never be part of any association. So if we were leaving this as-is (but I recommend changes to what this is returning below), I would say call this "TryFindIdentificationCodeProperty" and change the semantics (return a bool for success/failure, and an out property). But... read on.
| bool IsQueryableIdentificationCodeProperty(EntityProperty property) | ||
| { | ||
| return property.PropertyName.Equals("IdentificationCode") || | ||
| !(property.IsFromParent || property.IsAlreadyDefinedInCSharpEntityBaseClasses()); |
There was a problem hiding this comment.
IsAlreadyDefinedInCSharpEntityBaseClasses is currently unused by the API project and probably should only be available for use by CodeGen. It looks like IsPredefinedProperty is in use and accomplishes the same thing. Let's use that.
| } | ||
| } | ||
|
|
||
| private IDictionary<string, EntityProperty> GetIdentificationCodeParameters(EntityProperty entityIdentificationCodeProperty) |
There was a problem hiding this comment.
Do this work once per entity, and cache it by the root entity name in the top-level ConcurrentDictionary.
No description provided.