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

Association query #19

Closed
wants to merge 19 commits into from
Closed

Association query #19

wants to merge 19 commits into from

Conversation

Brooooooklyn
Copy link
Member

@Brooooooklyn Brooooooklyn commented Aug 8, 2017

Association query

  • refactor(PredicateProvider): PredicateProvider now support multiple tables, prepare for Association query
  • feat(PredicateProvider): support deep nested association query
  • feat(Database): support association query without association fields
  • test(PredicateProvider): add test for tableName missmatch in PredicateProvider
  • feat(Database): support nested predicate key
  • fix(Database#remove): stop passing Clause to PredicateProvider, instead of passing Predicate
  • chore: display report coverage after run coverage
  • feat(Database): warn if delete/remove/update all table
  • feat(utils/keys): use keys as alias for Object.keys
  • chore: yarn upgrade

Breaking Changes

  • feat(Database#get): restrict Associated Field position in Fields
  • feat(Database#get): Only navigation properties in query now valid

Related Issue:

resolve #14

@Brooooooklyn Brooooooklyn changed the title Association query [WIP]Association query Aug 8, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 96.4% when pulling eb765b1d323d0f3ae6d5ce714014cd3097ac38da on feature/association-query into ce214b2 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 96.4% when pulling 8c97a5bf0bd34d179e98a2bd0138b8eaa394ac07 on feature/association-query into ce214b2 on master.

@Saviio
Copy link
Contributor

Saviio commented Aug 8, 2017

这个是已经实现好了呀?

@Saviio Saviio requested review from chuan6 and Miloas August 8, 2017 12:24
@Saviio
Copy link
Contributor

Saviio commented Aug 9, 2017

@chuan6 @Miloas 之后你们先帮我review下这个PR啦~~

@chuan6
Copy link
Contributor

chuan6 commented Aug 9, 2017

@Saviio 嗯嗯,代码看过,但还没理解这个 PR 要达到的效果,今天再看看 👌

@Brooooooklyn
Copy link
Member Author

达到的效果是:

transform

{
  fields: [....],
  where: {
    'creator.gender': 'male' 
  }
}

to

SELECT 
  _id,
  Member.id,
  Member.name,
  Member.gender
FROM Message 
JOIN Member ON Message.creatorId = Member.id
WHERE Member.gender = 'male'

@@ -445,7 +449,8 @@ export class Database {
mainTable: table!
}
const { limit, skip } = clause
const provider = new PredicateProvider(table!, clause.where)
const tables = this.getAssoTablesDef(db, tableName, table)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Brooooooklyn 这边 tableName 和 table 可能不一致吗?

@chuan6
Copy link
Contributor

chuan6 commented Aug 9, 2017

@Brooooooklyn 我第一次注意到 createPredicate 这个函数,根据目前的实现,它的作用应该已经被现在的 PredicateProvider (可以处理空的 null 或 undefined 的 clause)代替了,还需要保留它吗?

@Brooooooklyn
Copy link
Member Author

可以保留,配合 tryCatch 作为:

try {
  new PredicateProvider(xxxxx).getPredicate()
  // success logic
} catch (e) {
  // error logic
}

的语法糖

@Brooooooklyn Brooooooklyn force-pushed the feature/association-query branch 2 times, most recently from 385b4b8 to 4ef39fe Compare August 9, 2017 10:31
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 96.28% when pulling 4ef39feb47ef967faaced5ca1755d3e79c178461 on feature/association-query into ce214b2 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 96.413% when pulling fa72de7d08cd9b45f2d53ed11108f4f8e0d630fa on feature/association-query into ce214b2 on master.

expect(result).to.deep.equal(innerTarget)
})

it.skip('should get association by deep nested Association query without association fields', function* () {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need some help here @Saviio

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

蛤? 有啥问题

@@ -747,6 +759,22 @@ export class Database {
}
}

private getAssoTablesDef(db: lf.Database, tableName: string, defaultTable: null | lf.schema.Table = null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getRelatedTables

@Brooooooklyn
Copy link
Member Author

Brooooooklyn commented Aug 9, 2017

我现在的做法是:

  1. 在 JoinInfo 中构建一个 tablesStructure, 它是这样的类型:
{
  [index: string]: { 
    table: lf.schema.Table
    contextName: string
  }
}
  1. 使 buildSelector 中 predicateProvider.getPredicate() 变成 lazy 的,即: 不是在 build joinInfo 的时候就 getPredicate,而是在从 joinInfo 中 build joinQuery 的时候再 getPredicate , 这个时候可以拿到整条查询语句的 tablesStructure,比如:
{
  'Task#1': {
     table: tableClass, // alias Task#1,
     contextName: 'Task#1'
   },
   'Task#1@project': {
      table: tableClass, // alias Project#1
      contextName: Project#1
    },
    'Project#1@organization': {
      table: tableClass, // alias Organization#1
      contextName: Organization#1
    }
}
  1. 让一条 get/upsert 语句中所有的 joinPredicate 和 wherePredicate 共用这个 tablesStructure, 然后到 PredicateProvider 中根据 meta 解析出 predicate

这样做有一个问题就是,当 query 的 predicate 中指定了某个关联的条件,但 fields 中并不包含这个关联的时候会构建不出 tablesStructure, 比如:

Database.get('Task', {
  fields:  ['_id'],
  where: {
    'project.organization._id': 'xxxx'
  }
})

具体表现就是 skip 掉的那个测试

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 96.413% when pulling 49f780161e544c70a93f43884125f359b9b04515 on feature/association-query into ce214b2 on master.

@Brooooooklyn Brooooooklyn force-pushed the feature/association-query branch 3 times, most recently from 4877b5e to cdafc68 Compare August 9, 2017 15:09
@Brooooooklyn Brooooooklyn changed the title [WIP]Association query Association query Aug 9, 2017
@Brooooooklyn
Copy link
Member Author

ready

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 96.266% when pulling 77e98bd on feature/association-query into a7e2496 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.504% when pulling 68fb8fe on feature/association-query into a7e2496 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 96.7% when pulling 541b338 on feature/association-query into a7e2496 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.4%) to 96.7% when pulling 1bdb3a0 on feature/association-query into a7e2496 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 96.487% when pulling ac32a6d on feature/association-query into a7e2496 on master.

})
})

it('should warn if build addition join info from predicate failed', function* () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo additional

@@ -1049,9 +1205,7 @@ export default describe('Database Testcase: ', () => {
const execRet1 = yield database.upsert<ProgramSchema>('Program', program)

yield database.delete<ProgramSchema>('Program', {
Copy link
Contributor

@Saviio Saviio Sep 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里接口不对称真的好吗

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok 的,get 才需要 fields,skip limit order 这些

Copy link
Contributor

@Saviio Saviio Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我明白,但是我还认为应该把接口设计的更对称,从形式上更接近

DELETE FROM table WHERE id > 0

这里的省略是一种"习惯的破坏"

@@ -539,21 +695,6 @@ export default describe('Database Testcase: ', () => {
})
})

it('should throw if only navigator was included in query', function* () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

咦,为什么删了

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

行为变了,默认会在所有的 associated query 上加上 pk

expect(result.project.organization._id).to.equal(innerTarget.project._organizationId)
})

it('should merge fields when get value by deep nested Association query with nested association fields', function* () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should merge fields if a nested association in the WHERE clause

Copy link
Contributor

@Saviio Saviio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

剩下的明天再看,脑子转不动了

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 96.487% when pulling d78d3a1 on feature/association-query into a7e2496 on master.

@@ -745,6 +785,115 @@ export class Database {
}
}

private buildTablesStructure(defaultTable: lf.schema.Table, aliasName?: string, tablesStruct: TablesStruct = Object.create(null)) {
Copy link
Contributor

@Saviio Saviio Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个函数叫 tablesStructure 有点奇怪....."复数的表"的结构,我觉得这系列函数名/变量的命名不如就叫tableStruct

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.532% when pulling ba7a13a on feature/association-query into a7e2496 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 96.532% when pulling 9e7db4a on feature/association-query into a7e2496 on master.

@chuan6 chuan6 added the Feature label Nov 17, 2017
@chuan6 chuan6 self-assigned this Nov 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Association query
5 participants