Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Update typeorm to v3 #2838

Merged
merged 3 commits into from
May 14, 2024

Conversation

yanguoyu
Copy link
Collaborator

When I want to use the find operator with the column that has the transform property. It can not work well. It has been fixed by typeorm/typeorm#9777. So I want to update typeorm to v3. But it brings in another issue typeorm/typeorm#9316.

Assume there exists a table table_a.

field_a field_b
'a1' 'b1'
'a2' null
  1. If we work with v2.
    const repo = connection.getRepository(TableA)
    // console []
    console.log(await repo.find({ field_b: undefined }))
   // console [ TableA { field_b: null, field_a: 'a2' } ]
    console.log(await repo.find({ field_b: null }))
    // console []
    console.log(await repo.createQueryBuilder('table_a').where({ field_b: undefined }).getMany())
    // console [ TableA { field_b: null, field_a: 'a2' } ]
    console.log(await repo.createQueryBuilder('table_a').where({ field_b: null }).getMany())
  1. If we work with v3.
    const repo = connection.getRepository(TableA)
    // console [TableA { field_b: 'b1', field_a: 'a1' }, TableA { field_b: null, field_a: 'a2' }]
    console.log(await repo.findBy({ field_b: undefined }))
    // console [TableA { field_b: 'b1', field_a: 'a1' }, TableA { field_b: null, field_a: 'a2' }]
    console.log(await repo.findBy({ field_b: null }))
    // console [TableA { field_b: null, field_a: 'a2' }], this is the recommended usage for v3, use IsNull to filter null field.
    console.log(await repo.findBy({ field_b: IsNull() }))
    // console []
    console.log(await repo.createQueryBuilder('table_a').where({ field_b: undefined }).getMany())
    // console []
    console.log(await repo.createQueryBuilder('table_a').where({ field_b: null }).getMany())

Currently, it's hard to check how many query has been affected, we can have a full test after the new UI release. Or
waiting for the https://github.com/typeorm/typeorm/issues/9316 conclusion then update typeorm. What do you think? @Keith-CY @WhiteMinds @homura

@WhiteMinds
Copy link
Contributor

Currently, it's hard to check how many query has been affected, we can have a full test after the new UI release. Or
waiting for the https://github.com/typeorm/typeorm/issues/9316 conclusion then update typeorm. What do you think?

There will likely be quite a bit of debate on this issue. I believe it is unlikely that TypeORM will revert to the old behavior unless some critical role of that old behavior is highlighted in the discussion.

BTW, I personally also prefer using approaches like isNULL, DATATYPE.NULL / DATATYPE.INTEGER etc to distinguish database types from JS native types, avoiding conflating their intent.

So I suggest we migrate to the new behavior, but do try to handle places that may be impacted, like hash: string | null | undefined, as much as possible at static code authoring time.

We could consider some actions to reduce the psychological burden of migrating from old to new behavior, such as temporarily (or permanently) removing null | undefined from the accepted value types in findOpts, using TS to provide cues, and enforcing no implicit any and checking explicit any to ensure proper type checking.

We could also consider patching TypeORM to report (just code locations) to something like Sentry when it receives null | undefined in findOpts, to quickly respond to unexpected errors.

@yanguoyu yanguoyu force-pushed the chore-update-typeorm branch 3 times, most recently from 5a4a3d2 to 4a9eab9 Compare January 10, 2024 07:17
@yanguoyu yanguoyu marked this pull request as ready for review January 10, 2024 07:17
@yanguoyu yanguoyu marked this pull request as draft January 10, 2024 07:19
@yanguoyu yanguoyu force-pushed the chore-update-typeorm branch 2 times, most recently from 89d7c5e to faf6c69 Compare January 24, 2024 01:55
@yanguoyu yanguoyu requested review from homura, devchenyan, FrederLu and silySuper and removed request for zhangyouxin January 24, 2024 01:56
@yanguoyu yanguoyu marked this pull request as ready for review January 24, 2024 01:56
@yanguoyu
Copy link
Collaborator Author

It's ready for review now. Please have a review. @Keith-CY @homura @devchenyan @WhiteMinds

@Keith-CY
Copy link
Collaborator

Conflicted

@Keith-CY
Copy link
Collaborator

Keith-CY commented Mar 28, 2024

@silySuper now focuses on other issues, I'd suggest merging this one first and verify it in a regression test.

@silySuper
Copy link
Collaborator

silySuper commented Mar 28, 2024

/package
Packaging for test is done in 8462022029. @silySuper

@devchenyan
Copy link
Collaborator

@silySuper
Copy link
Collaborator

1.When the remaining amount is 0,amend page shows 0 total amount,in expect it should be equal to amount in left.

截屏2024-03-29 10 24 06

2.In customized assets page ,when claim a histoty asset,it shows error(The same error shows when create asset account)
截屏2024-03-29 10 49 12

@yanguoyu
Copy link
Collaborator Author

https://github.com/yanguoyu/neuron/blob/d9a1d9b8e2ed22e6bcd173a1ab0a2c3aa40fdce0/packages/neuron-wallet/src/services/cell-local-info.ts#L31

image

@yanguoyu Regression testing found problems, not fully replaced.

I guess findOne also works well.

@yanguoyu
Copy link
Collaborator Author

1.When the remaining amount is 0,amend page shows 0 total amount,in expect it should be equal to amount in left.

截屏2024-03-29 10 24 06 2.In customized assets page ,when claim a histoty asset,it shows error(The same error shows when create asset account) 截屏2024-03-29 10 49 12

There may be some problems with your SQLite file. Could you rebuild the SQLite file with the release version and then test with this PR? Could you upload your SQLite file then I will check whether typeorm cannot connect your SQLite file.

@silySuper
Copy link
Collaborator

1.When the remaining amount is 0,amend page shows 0 total amount,in expect it should be equal to amount in left.
截屏2024-03-29 10 24 06
2.In customized assets page ,when claim a histoty asset,it shows error(The same error shows when create asset account) 截屏2024-03-29 10 49 12

There may be some problems with your SQLite file. Could you rebuild the SQLite file with the release version and then test with this PR? Could you upload your SQLite file then I will check whether typeorm cannot connect your SQLite file.

Another package I download yesterday can claim successfully.I do not change any file relate to sqlite manually.

截屏2024-03-29 14 51 21
2024-03-29.14.50.48.mov

This is my sqlite :
cells.zip

@yanguoyu
Copy link
Collaborator Author

Another package I download yesterday can claim successfully.I do not change any file relate to sqlite manually.

I'm not sure whether your local SQLite file is migrated by Neuron, Could you upload the main.log?

@silySuper
Copy link
Collaborator

main.log

@yanguoyu
Copy link
Collaborator Author

main.log

Besides, you can back up the SQLite files and then delete these files to test run by creating a new local file.

@silySuper
Copy link
Collaborator

main.log

Besides, you can back up the SQLite files and then delete these files to test run by creating a new local file.

Ok,this operation will sync from 0%,I am waiting it to sync 100%

@yanguoyu yanguoyu force-pushed the chore-update-typeorm branch from d9a1d9b to ba419a7 Compare March 29, 2024 09:47
@yanguoyu
Copy link
Collaborator Author

This PR https://github.com/typeorm/typeorm/pull/9751/files#diff-4cfd478e52f9edd89cf748e00253bfb16a436f300c5c93404739a51433edca1aR39-R47 change the query slow log to warn, so I copy the Logger from typeorm and change console.warn to console.info

@silySuper
Copy link
Collaborator

silySuper commented Apr 1, 2024

After open the neuron and turn off the computer at the same time for a long time,the log shows
[error] Sync:ChildProcess: query is slow: SELECT "tx"."hash" AS "tx_hash", "tx"."version" AS "tx_version", "tx"."cellDeps" AS "tx_cellDeps", "tx"."headerDeps" AS "tx_headerDeps", "tx"."witnesses" AS "tx_witnesses", "tx"."timestamp" AS "tx_timestamp", "tx"."blockNumber" AS "tx_blockNumber", "tx"."blockHash" AS "tx_blockHash", "tx"."description" AS "tx_description", "tx"."status" AS "tx_status", "tx"."createdAt" AS "tx_createdAt", "tx"."updatedAt" AS "tx_updatedAt", "tx"."confirmed" AS "tx_confirmed" FROM "transaction" "tx" WHERE "tx"."status" = ? -- PARAMETERS: ["pending"]
execution time: 48

@Keith-CY
Copy link
Collaborator

Any update on this PR

@Keith-CY
Copy link
Collaborator

Keith-CY commented May 10, 2024

/package
Packaging for test is done in 9030541323. @Keith-CY

@Keith-CY
Copy link
Collaborator

/package Packaging for test is done in 9030541323. @Keith-CY

Verified, could be merged first and tested again by @silySuper in regression tests.

@Keith-CY Keith-CY added this pull request to the merge queue May 14, 2024
Merged via the queue into nervosnetwork:develop with commit 9060738 May 14, 2024
9 checks passed
@yanguoyu yanguoyu deleted the chore-update-typeorm branch May 14, 2024 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants