Avoid hardcoded limit of 20 in projection evaluation - Githubissues

rs / rest-layer

REST Layer, Go (golang) REST API framework

http://rest-layer.io

MIT License

1.26k stars 114 forks source link

Avoid hardcoded limit of 20 in projection evaluation #249

Open Dragomir-Ivanov opened 5 years ago

Dragomir-Ivanov commented 5 years ago

Hi there, I want to propose a breaking API change, about moving resource.Conf in github.com/rs/rest-layer/resource to conf.Conf in github.com/rs/rest-layer/resource/conf. The reason for this is that I want to use resource configuration in schema/query package, that currently results in cyclic dependency.

More specifically I want to remove this constant: https://github.com/rs/rest-layer/blob/3f2cd5df9e8e6690c56f806710ef53048109c323/schema/query/projection_evaluator.go#L336 And use conf.PaginationDefaultLimit in connected resource.

smyrman commented 5 years ago

I see your point. Having it set to 20, I agree seam a bit arbitrary; it should probably rely on resources set for the specific resource.

However, I think the Conf is on the correct package (It's configuring a resource), and moving it to a separate package doesn't really make sense.

Besides, there are other issues with the current dependency tree that would make sense to solve at the same time. I.e. the query package is relying on resource.Item and resource.ItemList being translated to simple interface types containing the payload only, and define a separate Resource interface to glue this together.

It might be worthwhile reviewing the two packages resource and query, to find out if they could either be merged, or renamed & split in a different way so that the dependence cycle (including workarounds) is broken.

PS! The schema package should not need depend on the query package; we can solve dependencies with a schema instead of a query, and it would work equally well.

smyrman commented 5 years ago

I acknowledge the problem, but I think we need to come up with a better solution. I am thus going to close this issue now . Feel free to create a new issue with an alternate suggestion for restructuring.

Dragomir-Ivanov commented 5 years ago

Closing is okay, but I feel that this is major issue, and should be addressed soon, before any major package refactoring. Currently I am using a fork of rest-layer just because of this limit of 20, and more importantly I have removed validation for the reference resources ID, because:

I rely on Context and this validation uses just Context.TODO() which is critical for me.
This reference, actually references multiple resources, so its. Path is not valid. I can explain more my use case if necessary.

It could have been superb if this validation especially can be turned ON/OFF via config.

smyrman commented 5 years ago

Let's reopen/rename it then; just doing a bit of a cleanup in the issue tracker.

smyrman commented 5 years ago

I rely on Context and this validation uses just Context.TODO() which is critical for me.

Ok I reopened #192 as well; I suppose we still need to track it. It's really a bug, not an enchantment.

Dragomir-Ivanov commented 5 years ago

Thanks. I will think about it more, when I have some time.

smyrman commented 5 years ago

Here is a suggestion (breaking change) for solving the circular dependencies:

In package Schema, replace the Dependency query.Predicate field with Dependency *schema.Schema. See JSON Schema for documentation/help. This is a breaking change, but should be functionally mostly equivalent.
Move the query package from schema/query to resource/query.
Now, we can, as a bonus, remove the wrapper interface in the query package that replaces the direct use of resource.Item, as well as the private wrapper implementations in the rest package.

I would milestone this for a 0.3 release, if we can get #213 merged first so we can push out a release with that.

Dragomir-Ivanov commented 5 years ago

Cool, thanks! Will dig around when have some time. I need to polish my circular dependency hell skills with Go :)

smyrman commented 5 years ago

Actually query is also used within resource (my bad). But maybe a query interface could be defined for use in the resource package?

There are a few cases within the resource package where a query is actually defined though. Probably this approach also need some thought and cleaver work-arounds...

smyrman commented 5 years ago

Maybe the least invasive change would be to add a GetDefaultLimit to the existing Resource interface within the query package?

Then alter the existing resource wrapper types in the rest package to extract this from the .config.