pyeve / eve-sqlalchemy

SQLAlchemy data layer for Eve-powered RESTful APIs
http://eve-sqlalchemy.readthedocs.io
Other
234 stars 70 forks source link

Eve 0.7 support #178

Closed nicolaiarocci closed 6 years ago

nicolaiarocci commented 6 years ago

(this PR is probably to be considered a WIP, for @dkellner to review)

The necessary premise is that I have (very) limited knowledge of eve-sqlalchemy codebase. These changes might not be sufficient for a Eve 0.7 compatible release of the eve-sqlalchemy extension.

I wanted to see what was preventing eve-sqlalchemy from running on Eve 0.7. As it turns out, there are no major obstacles. With this commit all tests are passing with Eve<0.8, locally at least.

Next up would probably be a careful review of Eve 0.7 changelog, to see what new features (or fixes) need to be specifically addressed here.

dkellner commented 6 years ago

Great to hear it did not cause a lot of trouble! :) #136 is one of those issues that always got pushed ahead because of uncertainty how much work it's going to take.

I will release 0.6.0 first and then take a look at this. In the meantime: I've seen the ETag format changed, we should add a section to upgrading.rst including this and other breaking changes. Possibly referring to Eve's documentation is ok, too. Eve-SQLAlchemy is still just a data layer, people should be aware that upgrading to a new major version of the framework can cause breaking changes.

nicolaiarocci commented 6 years ago

I've seen the ETag format changed, we should add a section to upgrading.rst including this and other breaking changes

done.

dkellner commented 6 years ago

I'm really sorry for the huge delay but I'm still too busy for this. Next week I'm off for some days, I will look at it then.

dkellner commented 6 years ago

Possible problems

  1. Fix: ETag request headers which conform to RFC 7232/2.3 (double quoted value) are now properly processed. Addresses #794. ~> They are still stored without the quotes in the database, right?
  2. New: OPTIMIZE_PAGINATION_FOR_SPEED. ~> Does this work already with Eve-SQLAlchemy?

Further todos

  1. The upgrading section should link to Eve's breaking changes. I've seen some entries in the changelog (like "Change: Return 428 Precondition Required instead of a generic 403 Forbidden when the If-Match request header is missing.") that could affect clients and people should be aware of that before upgrading.
  2. Check if Eve's test structure is still like we expect for test case inheritance. E.g. if the test classes were split into smaller ones, we'll miss some tests.

I will look at (4) first.

dkellner commented 6 years ago
1. _Fix: ETag request headers which conform to RFC 7232/2.3 (double quoted value) are now properly processed. Addresses #794._ **~> They are still stored without the quotes in the database, right?**

Yes they are, so it should be backwards compatible.

2. _New: OPTIMIZE_PAGINATION_FOR_SPEED._ **~> Does this work already with Eve-SQLAlchemy?**

Well... it does not break anything and the tests for this new feature pass, but judging from the implementation it does not actually optimize anything in Eve-SQLAlchemy. I will create a separate issue for this.

4. Check if Eve's test structure is still like we expect for test case inheritance. E.g. if the test classes were split into smaller ones, we'll miss some tests.

Look good. Several new tests were introduced that pass and don't need any change to work with Eve-SQLAlchemy.

dkellner commented 6 years ago

@nicolaiarocci Apart from the question whether to pin the dependencies' versions or not, I think this is ready to be merged in master, thank you!

nicolaiarocci commented 6 years ago

@dkellner I went ahead and pinned the unpinned

dkellner commented 6 years ago

Merged after rebase.