ForestAdmin / lumber

Install Forest Admin in minutes.
https://www.forestadmin.com
MIT License
2.08k stars 106 forks source link

fix(analyzer): work around sequelize bugs on default values #554

Closed romain-gilliotte closed 3 years ago

romain-gilliotte commented 3 years ago

This PR works around sequelize bugs when retrieving default values from the database.

Each commit deals with different issues causing the bug

Note: As it differs quite a lot from one dialect of SQL to another I tested the default value extraction from the database only for Postgres. It was previously completely untested.

arnaudbesnier commented 3 years ago

Task linked: CU-f54mcu P3 - Lumber timestamp generation - invalide default value to timestamp

arnaudbesnier commented 3 years ago

Task linked: CU-8pnt1c P2 - Lumber - Lumber generates incorrect date default value

romain-gilliotte commented 3 years ago

First, thanks for taking so much time! I'll try to reply here to the remarks

  • The simpler the better, meaning don't mix things. If you want to refacto fine, but do a PR just with the refacto
  • Try to reduce as much thr work for the reviewer, this must be really simple for him. Here there is way too much info / line change for that fix. There is some refacto, some file changing, some renaming and ll of theme increase a lot the diff which takes times to review while I could may be have done it in a few minutes it took me hours 😉
  • Always fix only what you have in the title of your PR (So just the fix of your card), it look like you actually fixed something on the schema but I don't see the link with your PR 🤔
  • You can then of course do the ofther fixes in another PR put as I said before, the simpler the better ^^

Sorry, I did refactor in two locations! I think the bug was not easy to fix... Not my best choice for a first PR

My problem was that when querying information_schema, I need to know the schema, even when the user is not providing it explicitly (I needed that to get the default values). This was already the case for previous bug patches on this file:

I did not want to duplicate code, so I ended moving that in another location, and properly handling all cases. Before doing the whole schema extraction, I had some (manual...) tests failing on mysql.

  • Wording: We don't do shortcut in naming 😉 (example: cst => constant)

I still need to fix those on the SQL file

  • Always reproduce rhe issue in your test, so here you had an issue with CURRENT_TIMESTAMP, add it to the test 😉

I responded to that in another comment. I was also fixing default values like "timezone('utc'::text, now())" which are tested properly but CURRENT_TIMESTAMP proved more difficult.

  • And I think that's it for now 😅

It's a lot already 😅

forest-bot commented 3 years ago

:tada: This PR is included in version 4.0.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket: