elifesciences / elife-xpub

eLife is an open-access journal and technology provider that publishes promising research in the life and biomedical sciences. This is their implementation of a submission and peer review system based on Coko PubSweet and xPub.
https://elifesciences.org/
MIT License
32 stars 5 forks source link

Investigate the alternatives to Objection #2192

Closed diversemix closed 5 years ago

diversemix commented 5 years ago

To avoid the "Sunk cost fallacy" after several major issues with the objection library we have decided to investigate using alternatives.

This should include alternative ORM's as well as using something like knex to directly connect with the database. The aim is to have a clear separation of responsibility between database operations and the rest of the code.

diversemix commented 5 years ago

Turns out someone else has done the job here: https://docs.google.com/spreadsheets/d/1sSbY8SLWA9lvvnTHX41t0TVPXwZ4A4ddO_KJMa20fA4/edit#gid=0

Top 5 are:

Another factor - maybe part of #2101 - is whether we want to stick to the Active Record pattern or move more to the Repository Pattern...

So finally a choice could be to use a query builder like knex (https://knexjs.org/) to take the strain out from building sql queries but leave db operations flexible to specify for the rest of the app to use.

diversemix commented 5 years ago

My recommendation is either:

diversemix commented 5 years ago

Discussion going on in the office here... The idea is as follows:

We all swarm and split into 2 groups.

  1. Use sequelize to generate an alternative ORM to BaseModel for just component-model-file
  2. Use knex to generate an alternative to BaseModel using a repo pattern for just component-model-file

The groups then review each others PR and we then jointly come to a decision on which route we take. (not expected to take more than 2 days)

/cc @hdrury1 @BlueReZZ

jure commented 5 years ago

Would be good to have the issues you encountered listed here as a reference, to guide discussions upstream as well!

Good luck!

hdrury1 commented 5 years ago

Happy with the plan. Let's make sure we time box this- I'd like to have a final decision by the time we move on to the next priority milestone early next week.

diversemix commented 5 years ago

Would be good to have the issues you encountered listed here as a reference, to guide discussions upstream as well!

Good luck!

@jure I wouldn't like to comb through all our issues to flag the ones whose root cause has been objection. But the team and myself have sunk a lot of time in understanding it and fixing issues as a result of objection's side effects (like saving or not saving relations, in fact we have a whole suite of tests dedicated to understanding this). There have been several significant problems involving objection ... take the issue we had in production on Monday - which prevented data being persisted in the form - a potentially huge and embarrassing problem. Looking at the fix you can see that the design of objection does not really mitigate developer errors. All these issues leaves me (and the team) worried about its lack of maturity for production grade systems.

jure commented 5 years ago

Makes sense! That issue you linked to indeed isn't a shining example of developer friendliness. I've made an effort to improve it with strict type checking for all graph options over here https://github.com/Vincit/objection.js/pull/1391, as I also found one instance in PubSweet's tests that was guilty of misusing the options (though the test passed, because the option currently just gets !!-ed in objection.js)

I'm already excited for what ORM alternatives you find out there and how it'll all look when you take them for a spin! We tried to balance freedom and handholding by choosing Objection.js, which is very well tested with a biggish community, modern features and a solid list of users in production (https://github.com/Vincit/objection.js/issues/1069), but I can understand and appreciate that there likely isn't a single option that will fit all PubSweet use-cases and team preferences/expectations exceptionally well.

diversemix commented 5 years ago

Vote taken https://elifesciences.slack.com/archives/C6LKMFK5Z/p1561631340023600

knex is the winner (unanimously) - this will form part of our all-day-discussion tomorrow.