[feature] defined order of properties in API Form #1491
Conversation
|
Deployed review-app can be viewed at https://review-1491.violet-test.net |
|
Deployed review-app can be viewed at https://review-1491.violet-test.net |
donrestarone
changed the title
|
Deployed review-app can be viewed at https://review-1491.violet-test.net |
| @@ -97,7 +97,16 @@ class ApiNamespace < ApplicationRecord | |||
| else | |||
| self.none | |||
| end | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
@Pralish is this change necessary?
There was a problem hiding this comment.
@donrestarone , it's not necessary. The indentation was one space ahead, so fixed it.
| def up | ||
| remove_index :api_namespaces, name: "index_api_namespaces_on_properties" | ||
|
|
||
| change_column :api_namespaces, :properties, :json |
There was a problem hiding this comment.
@Pralish I'm worried this is going to lock up the table for a long time in big apps, causing requests to hang while the DB changes in the background
Can you add strong_migrations https://github.com/ankane/strong_migrations and see what it says?
There was a problem hiding this comment.
@donrestarone, The change is on api_namespaces table. I don't think we have a lot of api_namespaces (less than 50 per app in my opinion).
Having said that, I used the strong_migrations gem and this was the response:
== 20230323050938 ChangePropertiesInApiNamespacesFromJsonbToJson: migrating ===
-- remove_index(:api_namespaces, {:name=>"index_api_namespaces_on_properties"})
-> 0.0019s
rails aborted!
StandardError: An error has occurred, this and all later migrations canceled:
=== Dangerous operation detected #strong_migrations ===
Changing the type of an existing column blocks reads and writes
while the entire table is rewritten. A safer approach is to:
1. Create a new column
2. Write to both columns
3. Backfill data from the old column to the new column
4. Move reads from the old column to the new column
5. Stop writing to the old column
6. Drop the old column
There was a problem hiding this comment.
@Pralish yeah that was my fear. Can we add strong migrations to the code base, ensure it runs in CI and then tackle this issue with their suggestion?
donrestarone
added
do not merge
and removed
deploy-review-app
Passing UAT on Testnet ✅
labels
|
@Pralish we are pivoting this task into supporting multiple form representations. This way we don't need to do 5 deployments and we get to keep the JSONB structure for entity representation |
Addresses:
#555
#1381
Demo:
Screen.Recording.2023-03-23.at.7.01.06.PM.mov