Allow sparse fieldsets parameter to be dashed case

[dejarp]

https://jsonapi.org/format/#fetching-sparse-fieldsets

The examples in the spec suggest that the fields are identified by the JSON property (dashed) name. This change should enable that use case and remain backwards compatible with the Pascal case.

[joukevandermaas]

For my understanding, when a person does these queries, should they expect the same result?

?fields[corporation]=name,NumberOfEmployees
?fields[corporation]=name,numberofemployees
?fields[corporation]=name,NUMBEROFEMPLOYEES
?fields[corporation]=name,numberOfEmployees
?fields[corporation]=name,number-of-employees
?fields[corporation]=name,number_of_employees

Looking at this change, I don't think that will work, correct? Maybe for this scenario we don't want to use the property name converter but instead do the following:

.Replace("_", "").Replace("-", "").ToUpperInvariant()

This could of course be an extension method, perhaps ToComparablePropertyName.

What do you think?

[dejarp]

It's unclear from the spec which input property formats should be supported. imo, it should only accept dashed form, but it already accepts PascalCase, so for reverse compatibility I think it should support those 2, but it's less important to support other formats besides those 2. Again, this is just my opinion and not part of the spec. ToComparablePropertyName may be quite difficult to implement, especially for all caps and all lowercase due to difficulty of computing word boundaries.

[joukevandermaas]

@dejarp I understand the argument, but I'm more or less convinced someone will run into this and ask to support the other types as well (based on experience :smile:). I do wish the spec was a bit more strict on this point because there's too much left to guess for the implementers. Either way I have always erred on the side of allowing more when it comes to this stuff in Saule.

As for ToComparablePropertyName I think the implementation I suggested will cover almost everyone's use cases, if it does not we could fix that later. Since it looks like in the code we only ever compare, not search, I think if we call that on both sides (the property name and the field name) it should work out.

If you strongly disagree with the above please let me know, I'm very open to being convinced, but for now it seems that being lenient is the more future-proof solution.

[dejarp]

I strongly believe that the format returned in the response should match the format used in the query string in all cases. Over the long term I think this will promote intuitive usage. That being said, it could support all formats (for exceptional situations or preferences) via a configuration section of some sort (attribute maybe, so that it can be configure per endpoint). If this route were chosen, would deprecate PascalCase from default accepted formats in next major release (consumers would only need to add that format attribute to all endpoints they need to be reverse compatible, which may be a large number but simple to add).

[edward-rosado]

with sparsefieldsets the api consumer should be able to look at the response object and determine which ones he/she wants to use. I agree with @dejarp that at the very least this should for what is returned. If other casing is supported that is also fine.

[edward-rosado]

@joukevandermaas as of right now, lower cased dashed property names do not work with sparse fieldsets which in itself is a problem that should be fixed.

While I agree that other formats may want to be added in time, I dont agree that the framework should be forced to support them all. By default doing the most basic case and allowing the implementor to override should suffice.

Right now the caller has to do some arbitrary conversion from lower case dashed to Pascal Case and they wouldnt inheritly know that from the response being returned to them.

[joukevandermaas]

@edward-rosado the problem is not whatever feature or bug fix, but the fact that I strongly expect the feature request will come to change this, and by then it will be a breaking change. I don't want to hack around it then, if we can solve the issue now.

In other places we are lenient with URL provided parameters so I think we should be here too. Going back on that leniency is also a breaking change we cannot make anymore. Basically philosophical arguments just don't apply in this case, we have to be practical which means we have to be allow the same options.

@edward-rosado I hope this argument makes sense (I tried to make it above as well). I don't really disagree with anything you or @dejarp have said above, I am just thinking ahead a little bit to keep everything consistent and easy to use.

[dejarp]

@joukevandermaas @edward-rosado updated PR to use ToComparablePropertyName and added additional test cases.

[joukevandermaas]

Thanks!