-
-
Notifications
You must be signed in to change notification settings - Fork 158
Nested sort #416
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
Nested sort #416
Changes from 9 commits
94fdff2
8d46f52
2f3e5d1
b04ee1f
d63ff38
487bef9
18014f6
61dacea
188de7d
43071ed
7e9fc76
4102288
2e304cc
d97affd
47d4fcd
cc3564d
5b6e614
95b3acc
5e1db73
04172a4
0be3300
d1bc2cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
using System; | ||
using System.Linq; | ||
using JsonApiDotNetCore.Models; | ||
using JsonApiDotNetCore.Services; | ||
|
@@ -6,32 +7,25 @@ namespace JsonApiDotNetCore.Internal.Query | |
{ | ||
public class AttrFilterQuery : BaseFilterQuery | ||
{ | ||
private readonly IJsonApiContext _jsonApiContext; | ||
|
||
public AttrFilterQuery( | ||
IJsonApiContext jsonApiContext, | ||
FilterQuery filterQuery) | ||
: base(jsonApiContext, | ||
null, | ||
filterQuery.Attribute, | ||
filterQuery.Value, | ||
filterQuery.OperationType) | ||
{ | ||
_jsonApiContext = jsonApiContext; | ||
|
||
var attribute = GetAttribute(filterQuery.Attribute); | ||
|
||
if (attribute == null) | ||
if (Attribute == null) | ||
throw new JsonApiException(400, $"'{filterQuery.Attribute}' is not a valid attribute."); | ||
|
||
if (attribute.IsFilterable == false) | ||
throw new JsonApiException(400, $"Filter is not allowed for attribute '{attribute.PublicAttributeName}'."); | ||
if (Attribute.IsFilterable == false) | ||
throw new JsonApiException(400, $"Filter is not allowed for attribute '{Attribute.PublicAttributeName}'."); | ||
|
||
FilteredAttribute = attribute; | ||
PropertyValue = filterQuery.Value; | ||
FilterOperation = GetFilterOperation(filterQuery.Operation); | ||
FilteredAttribute = Attribute; | ||
} | ||
|
||
public AttrAttribute FilteredAttribute { get; } | ||
public string PropertyValue { get; } | ||
public FilterOperations FilterOperation { get; } | ||
|
||
private AttrAttribute GetAttribute(string attribute) => | ||
_jsonApiContext.RequestEntity.Attributes.FirstOrDefault(attr => attr.Is(attribute)); | ||
[Obsolete("Use " + nameof(Attribute) + " property of " + nameof(BaseAttrQuery) + "class. This property is shared for all AttrQuery and RelatedAttrQuery (filter,sort..) implementations.")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's simplify the language here a bit, maybe: [Obsolete("Use " + nameof(BaseAttrQuery.Attribute) + " insetad.")] I'm not sure the rest adds any extra value and increases build output noise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
public AttrAttribute FilteredAttribute { get; set; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
using System; | ||
using System.Linq; | ||
using JsonApiDotNetCore.Models; | ||
using JsonApiDotNetCore.Services; | ||
|
||
namespace JsonApiDotNetCore.Internal.Query | ||
{ | ||
public class AttrSortQuery : BaseAttrQuery | ||
{ | ||
public AttrSortQuery( | ||
IJsonApiContext jsonApiContext, | ||
SortQuery sortQuery) | ||
:base(jsonApiContext, null, sortQuery.Attribute) | ||
{ | ||
if (Attribute == null) | ||
throw new JsonApiException(400, $"'{sortQuery.Attribute}' is not a valid attribute."); | ||
|
||
if (Attribute.IsSortable == false) | ||
throw new JsonApiException(400, $"Sort is not allowed for attribute '{Attribute.PublicAttributeName}'."); | ||
|
||
Direction = sortQuery.Direction; | ||
} | ||
|
||
public SortDirection Direction { get; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
using JsonApiDotNetCore.Models; | ||
using JsonApiDotNetCore.Services; | ||
using System.Linq; | ||
|
||
namespace JsonApiDotNetCore.Internal.Query | ||
{ | ||
/// <summary> | ||
/// Abstract class to make available shared properties of all query implementations | ||
/// It elimines boilerplate of providing specified type(AttrQuery or RelatedAttrQuery) | ||
/// while filter and sort operations and eliminates plenty of methods to keep DRY principles | ||
/// </summary> | ||
public abstract class BaseAttrQuery | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All shared properties (Attribute and Relationship) are located here. All implementations should use these default properties to keep DRY. Also this is the main class that can be cached for filter, sort and fields methods and make always one instance of Attribute and Relationship classes (in the future) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah I see, generally I only use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it depends on use case. I was inspired by EF approach and it's table inheritance - abstract base classes holds shared properties for all descendants and prevents creating new instances directly https://www.learnentityframeworkcore.com/inheritance/table-per-hierarchy |
||
{ | ||
private readonly IJsonApiContext _jsonApiContext; | ||
|
||
public BaseAttrQuery(IJsonApiContext jsonApiContext, string relationship, string attribute) | ||
{ | ||
_jsonApiContext = jsonApiContext; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's depend on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I don't understand, but all the time AttrFilterQuery and RelatedAttrQuery depends on IJsonApiContext, so why IContextGraph? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, you're correct 😞 , the idea was to only depend on the interface that you actually need. #253 elaborates on this a bit more. |
||
if (string.IsNullOrEmpty(relationship)) | ||
Attribute = GetAttribute(attribute); | ||
else | ||
{ | ||
Relationship = GetRelationship(relationship); | ||
Attribute = GetAttribute(Relationship, attribute); | ||
} | ||
|
||
} | ||
|
||
public AttrAttribute Attribute { get; } | ||
public RelationshipAttribute Relationship { get; } | ||
public bool IsAttributeOfRelationship => Relationship != null; | ||
|
||
public string GetPropertyPath() | ||
{ | ||
if (IsAttributeOfRelationship) | ||
return string.Format("{0}.{1}", Relationship.InternalRelationshipName, Attribute.InternalAttributeName); | ||
else | ||
return Attribute.InternalAttributeName; | ||
} | ||
|
||
private AttrAttribute GetAttribute(string attribute) | ||
=> _jsonApiContext.RequestEntity.Attributes.FirstOrDefault(attr => attr.Is(attribute)); | ||
|
||
private RelationshipAttribute GetRelationship(string propertyName) | ||
=> _jsonApiContext.RequestEntity.Relationships.FirstOrDefault(r => r.Is(propertyName)); | ||
|
||
private AttrAttribute GetAttribute(RelationshipAttribute relationship, string attribute) | ||
{ | ||
var relatedContextExntity = _jsonApiContext.ContextGraph.GetContextEntity(relationship.Type); | ||
return relatedContextExntity.Attributes | ||
.FirstOrDefault(a => a.Is(attribute)); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,24 @@ | ||
using JsonApiDotNetCore.Models; | ||
using JsonApiDotNetCore.Services; | ||
using System; | ||
|
||
namespace JsonApiDotNetCore.Internal.Query | ||
{ | ||
public class BaseFilterQuery | ||
public class BaseFilterQuery : BaseAttrQuery | ||
{ | ||
public BaseFilterQuery( | ||
IJsonApiContext jsonApiContext, | ||
string relationship, | ||
string attribute, | ||
string value, | ||
FilterOperations op) | ||
: base(jsonApiContext, relationship, attribute) | ||
{ | ||
PropertyValue = value; | ||
FilterOperation = op; | ||
} | ||
|
||
[Obsolete("To resolve operation use enum typed " + nameof(FilterQuery.OperationType) + " property of "+ nameof(FilterQuery) +" class")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simplify: [Obsolete("Use " + nameof(FilterQuery.OperationType) + " instead.")] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
protected FilterOperations GetFilterOperation(string prefix) | ||
{ | ||
if (prefix.Length == 0) return FilterOperations.eq; | ||
|
@@ -13,5 +28,8 @@ protected FilterOperations GetFilterOperation(string prefix) | |
|
||
return opertion; | ||
} | ||
|
||
public string PropertyValue { get; } | ||
public FilterOperations FilterOperation { get; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
using JsonApiDotNetCore.Models; | ||
using JsonApiDotNetCore.Services; | ||
using System; | ||
using System.Linq; | ||
|
||
namespace JsonApiDotNetCore.Internal.Query | ||
{ | ||
public abstract class BaseQuery | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because concept is same as BaseAttrQuery, that only holds info about attributes, relationship etc. I don't know about use case, when client should create new instance of this BaseQuery, only descendant class (FilterQuery, SortQuery). Or do you have some idea where to use it without abstract? |
||
{ | ||
public BaseQuery(string attribute) | ||
{ | ||
var properties = attribute.Split(QueryConstants.DOT); | ||
if(properties.Length > 1) | ||
{ | ||
Relationship = properties[0]; | ||
Attribute = properties[1]; | ||
} | ||
else | ||
Attribute = properties[0]; | ||
} | ||
|
||
public string Attribute { get; } | ||
public string Relationship { get; } | ||
public bool IsAttributeOfRelationship => Relationship != null; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,23 +1,40 @@ | ||
using System; | ||
using JsonApiDotNetCore.Extensions; | ||
using JsonApiDotNetCore.Models; | ||
|
||
namespace JsonApiDotNetCore.Internal.Query | ||
{ | ||
public class FilterQuery | ||
public class FilterQuery : BaseQuery | ||
{ | ||
[Obsolete("Use constructor with FilterOperations operationType paremeter. Filter operation should be provided " + | ||
"as enum type, not by string.")] | ||
public FilterQuery(string attribute, string value, string operation) | ||
:base(attribute) | ||
{ | ||
Attribute = attribute; | ||
Key = attribute.ToProperCase(); | ||
Key = attribute.ToProperCase(); | ||
Value = value; | ||
Operation = operation; | ||
|
||
Enum.TryParse(operation, out FilterOperations opertion); | ||
OperationType = opertion; | ||
} | ||
|
||
public FilterQuery(string attribute, string value, FilterOperations operationType) | ||
: base(attribute) | ||
{ | ||
Key = attribute.ToProperCase(); | ||
Value = value; | ||
Operation = operationType.ToString(); | ||
OperationType = operationType; | ||
} | ||
|
||
[Obsolete("Key has been replaced by '" + nameof(Attribute) + "'. Members should be located by their public name, not by coercing the provided value to the internal name.")] | ||
public string Key { get; set; } | ||
public string Attribute { get; } | ||
public string Value { get; set; } | ||
[Obsolete("Operation has been replaced by '" + nameof(OperationType) + "'. OperationType is typed enum value for Operation property. This should be default property for providing operation type, because of unsustainable string (not typed) value.")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Obsolete("Use '" + nameof(OperationType) + "' instead.")] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
public string Operation { get; set; } | ||
public bool IsAttributeOfRelationship => Attribute.Contains("."); | ||
|
||
public FilterOperations OperationType { get; set; } | ||
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
using System; | ||
using System.Linq; | ||
using JsonApiDotNetCore.Models; | ||
using JsonApiDotNetCore.Services; | ||
|
@@ -6,45 +7,28 @@ namespace JsonApiDotNetCore.Internal.Query | |
{ | ||
public class RelatedAttrFilterQuery : BaseFilterQuery | ||
{ | ||
private readonly IJsonApiContext _jsonApiContext; | ||
|
||
public RelatedAttrFilterQuery( | ||
IJsonApiContext jsonApiContext, | ||
FilterQuery filterQuery) | ||
:base(jsonApiContext, filterQuery.Relationship, filterQuery.Attribute, filterQuery.Value, filterQuery.OperationType) | ||
{ | ||
_jsonApiContext = jsonApiContext; | ||
if (Relationship == null) | ||
throw new JsonApiException(400, $"{filterQuery.Relationship} is not a valid relationship on {jsonApiContext.RequestEntity.EntityName}."); | ||
|
||
var relationshipArray = filterQuery.Attribute.Split('.'); | ||
var relationship = GetRelationship(relationshipArray[0]); | ||
if (relationship == null) | ||
throw new JsonApiException(400, $"{relationshipArray[1]} is not a valid relationship on {relationshipArray[0]}."); | ||
|
||
var attribute = GetAttribute(relationship, relationshipArray[1]); | ||
if (attribute == null) | ||
if (Attribute == null) | ||
throw new JsonApiException(400, $"'{filterQuery.Attribute}' is not a valid attribute."); | ||
|
||
if (attribute.IsFilterable == false) | ||
throw new JsonApiException(400, $"Filter is not allowed for attribute '{attribute.PublicAttributeName}'."); | ||
if (Attribute.IsFilterable == false) | ||
throw new JsonApiException(400, $"Filter is not allowed for attribute '{Attribute.PublicAttributeName}'."); | ||
|
||
FilteredRelationship = relationship; | ||
FilteredAttribute = attribute; | ||
PropertyValue = filterQuery.Value; | ||
FilterOperation = GetFilterOperation(filterQuery.Operation); | ||
FilteredRelationship = Relationship; | ||
FilteredAttribute = Attribute; | ||
} | ||
|
||
[Obsolete("Use " + nameof(Attribute) + " property. It's shared for all implementations of BaseAttrQuery(better sort, filter) handling")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Obsolete("Use " + nameof(Attribute) + " instead.")] and similarly below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
public AttrAttribute FilteredAttribute { get; set; } | ||
public string PropertyValue { get; set; } | ||
public FilterOperations FilterOperation { get; set; } | ||
public RelationshipAttribute FilteredRelationship { get; } | ||
|
||
private RelationshipAttribute GetRelationship(string propertyName) | ||
=> _jsonApiContext.RequestEntity.Relationships.FirstOrDefault(r => r.Is(propertyName)); | ||
|
||
private AttrAttribute GetAttribute(RelationshipAttribute relationship, string attribute) | ||
{ | ||
var relatedContextExntity = _jsonApiContext.ContextGraph.GetContextEntity(relationship.Type); | ||
return relatedContextExntity.Attributes | ||
.FirstOrDefault(a => a.Is(attribute)); | ||
} | ||
[Obsolete("Use " + nameof(Relationship) + " property. It's shared for all implementations of BaseAttrQuery(better sort, filter) handling")] | ||
public RelationshipAttribute FilteredRelationship { get; set; } | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
using System; | ||
using System.Linq; | ||
using JsonApiDotNetCore.Models; | ||
using JsonApiDotNetCore.Services; | ||
|
||
namespace JsonApiDotNetCore.Internal.Query | ||
{ | ||
public class RelatedAttrSortQuery : BaseAttrQuery | ||
{ | ||
public RelatedAttrSortQuery( | ||
IJsonApiContext jsonApiContext, | ||
SortQuery sortQuery) | ||
:base(jsonApiContext, sortQuery.Relationship, sortQuery.Attribute) | ||
{ | ||
if (Relationship == null) | ||
throw new JsonApiException(400, $"{sortQuery.Relationship} is not a valid relationship on {jsonApiContext.RequestEntity.EntityName}."); | ||
|
||
if (Attribute == null) | ||
throw new JsonApiException(400, $"'{sortQuery.Attribute}' is not a valid attribute."); | ||
|
||
if (Attribute.IsSortable == false) | ||
throw new JsonApiException(400, $"Sort is not allowed for attribute '{Attribute.PublicAttributeName}'."); | ||
|
||
Direction = sortQuery.Direction; | ||
} | ||
|
||
public SortDirection Direction { get; } | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason we can't just pass
filterQuery
rather than expanding it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, first solution was different ( no class BaseAttrQuery). Now I can pass filterQuery and BaseAttrQuery will have BaseQuery in constructor parameters. Good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed