search

ActiveChooN / gin-gorm-filter

Filter GORM query with query params
MIT License
16 stars 13 forks source link

Handling an error from c.BindQuery(&params) properly #16

Open ildargafarov opened 3 weeks ago

ildargafarov commented 3 weeks ago

Hi! I think it will be great if it allows to handle an error after calling c.BindQuery(&params). For example, I need to return 400 Bad Request if any query parameter is invalid. My proposal is to add another func like FilterByQueryParams(params QueryParams, config int) func(db *gorm.DB) *gorm.DB and make queryParams public. At first glance these changes fit with current API. I can prepare PR, but decided to discuss before writting any code

ashvinsharma commented 3 weeks ago

Yes please

ActiveChooN commented 3 weeks ago

Hi @ildargafarov! Thanks for your issue! It's a good point that there should be proper error handling. But instead of a new function, I suggest using the db.AddError() function. It looks quite convenient for users to handle it that way and should be quite simple from code perspective. Do you mind creating a PR?

ildargafarov commented 3 weeks ago

Hi @ActiveChooN Thanks for your answer. I disagree with you suggestion, to my regret. I think there is nothing in common between validating API parameters and ORM errors. From my point of view, these errors don't relate to DB level. I understand that you strive for retaining the current library's API, but it had incorrect design from the beginning. I have chosen to implement my own solution, so I won't create a PR. Sorry. By the way, this was not only one thing that have influenced my decision. I have found that the lib doesn't work well with integer, bool and nil values. This is significant issue for me

ActiveChooN commented 2 weeks ago

@ildargafarov, thank you for your reply. After looking at your proposal a second time, I can actually see more benefits in it. It can make it easy to use lib with different params source (pagination with X-Pagination headers) or names (limit and offset) or even not rely on gorm lib. And I don't mind breaking API specs before a major release. So feel free to create PR with your proposed solution. Also feel free to create any issues related to non-string values.

ildargafarov commented 2 weeks ago

Ok, I'll push a PR soon