Add Indexes to Schema API #4240

dplewis · 2017-10-06T06:27:07Z

Adds an optional indexes field to the Schema API.

Just a first pass though to make sure we're on the right track.

Thoughts?

codecov · 2017-10-06T12:30:44Z

Codecov Report

Merging #4240 into 3.x will increase coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##              3.x    #4240      +/-   ##
==========================================
+ Coverage   92.45%   92.57%   +0.12%     
==========================================
  Files         118      118              
  Lines        8218     8341     +123     
==========================================
+ Hits         7598     7722     +124     
+ Misses        620      619       -1

Impacted Files	Coverage Δ
src/Routers/SchemasRouter.js	`97.91% <100%> (ø)`	⬆️
src/Controllers/SchemaController.js	`97.11% <100%> (+0.11%)`	⬆️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`94.5% <100%> (+1.54%)`	⬆️
...rc/Adapters/Storage/Mongo/MongoSchemaCollection.js	`96.15% <100%> (+0.2%)`	⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`96.93% <100%> (+0.13%)`	⬆️
src/Controllers/DatabaseController.js	`94.62% <100%> (-0.19%)`	⬇️
src/RestWrite.js	`93.46% <0%> (+0.36%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c36400a...102af94. Read the comment docs.

dplewis · 2017-10-06T16:45:25Z

@vitaly-t Can you tell me if I used connections correctly on Postgres?

vitaly-t · 2017-10-06T16:54:50Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

+  }
+
+  getIndexes(className) {
+    const qs = `SELECT * FROM pg_indexes WHERE tablename = '${className}'`;


Do not use ES6 strings to format values, replace this with:

const qs = 'SELECT * FROM pg_indexes WHERE tablename = ${className}'; return this._client.any(qs, {className});

This is just one example, u have other places that can be improved the same ;)

vitaly-t · 2017-10-06T16:58:07Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

+
+  dropIndexes(className, indexes, conn) {
+    conn = conn || this._client;
+    return conn.tx(t => {


Much simpler code:

return conn.tx(t => t.batch(indexes.map(i => t.none('DROP INDEX $1', i))));

vitaly-t · 2017-10-06T17:01:26Z

@dplewis it appears to be correct, at first sight anyway.

I left some comments there for you, as examples of:

need to avoid use of ES6 string to format values, it is dangerous
some transactions/tasks can be much simpler ;)

dplewis · 2017-10-06T17:03:36Z

@vitaly-t Thanks a lot. Why is it dangerous to use ES6 Strings?

vitaly-t · 2017-10-06T17:04:18Z

@dplewis ES6 strings know nothing about how to correctly escape values for PostgreSQL, only the pg-promise formatting engine knows it :)

vitaly-t · 2017-10-06T17:08:04Z

@dplewis worth noting that some of the code there can probably be improved due to the following recent update in the driver: https://github.com/vitaly-t/pg-promise/releases/tag/v.6.10.0

dplewis · 2017-10-06T17:15:31Z

@vitaly-t I know the formatting but I have a bad habit of not using pg-promise the way it was intended.

vitaly-t · 2017-10-06T17:20:48Z

Agreed, it is a bad habit :)

dplewis · 2017-10-06T22:23:49Z

@flovilmart can you look this over?

flovilmart · 2017-10-07T17:03:41Z

Kinda busy today, gotta have a look tomorrow :)

vitaly-t · 2017-10-10T19:38:05Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

@@ -1397,6 +1451,30 @@ export class PostgresStorageAdapter {
        console.error(error);
      });
  }
+
+  createIndexes(className, indexes, conn) {
+    conn = conn || this._client;


Better implementation:

return (conn || this._client).tx(t => t.batch(indexes.map(i => { return t.none(`CREATE INDEX $1:name ON $2:name($3:name)`, [i.name, className, i.key]); })));

Note that filter :name is very smart...

Filter :name, for SQL Names, has a special logic to it, and it can be:

a simple text string, which is normal, to represent a single column name

string * (asterisks), which is detected automatically

array of strings, in which case it is considered a list of column names

any object, to pull all property names automatically.

This is why in our case we can simply pass in an object, and :name will automatically pull all property names from it.

I didn't know about the property names. Thats super helpful.

I really should read the pg-promise documentation again

@dplewis I have just updated documentation for SQL Names 😉

vitaly-t · 2017-10-11T00:52:29Z

I might do another round of review after this PR is merged along with the pg-promise upgraded to version 7.x

acinader

much needed!

flovilmart · 2017-10-20T17:18:28Z

@dplewis quick question, does it reads/loads all existing indexes?

dplewis · 2017-10-20T17:24:40Z

@flovilmart No, only if someone has a compound index existing for full text search. Should we load all indexes?

flovilmart · 2017-10-20T17:35:00Z

That would be neat, this way we bridge fully compared to the schema don't you think?

dplewis · 2017-10-20T18:07:02Z

I agree. Should we get the indexes on initialization?

flovilmart · 2017-10-20T22:54:44Z

Seems like a good idea!

montymxb

Much better with the history, this looks good now!

Before I give this a thumbs-up, and I'm fairly certain, but this does load up pre-existing indexes, compound or not, on startup right? I'm pretty sure that it got added in but I wanted to be clear that it is present & accounted for.

dplewis · 2017-11-16T03:44:35Z

@montymxb Indexes are available on startup but they aren't transformed. I'll have to add a transform for every index.

montymxb · 2017-11-16T19:45:07Z

@dplewis so long as we don't create another issue like #4212 . I think we're good here though.

montymxb

Alright, @flovilmart we're ready here. Any thoughts before we merge this?

dplewis · 2017-11-16T20:00:41Z

I found a few issues I’ll submit a fix for shortly

flovilmart · 2017-11-16T20:05:23Z

Awesome thanks @dplewis !

montymxb · 2017-11-21T21:31:20Z

@dplewis just marking this as WIP until you finish the stuff you're working on .

montymxb · 2017-11-22T00:26:37Z

spec/schemas.spec.js

@@ -2336,8 +2348,12 @@ describe('schemas', () => {
        headers: masterKeyHeaders,
        json: true,
      }, (error, response, body) => {
+        console.log(body);


log here should be removed

montymxb · 2017-11-22T00:26:50Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

@@ -1251,6 +1254,7 @@ export class PostgresStorageAdapter {
  }

  find(className, schema, query, { skip, limit, sort, keys }) {
+    console.log('find');


dplewis · 2017-11-22T14:43:50Z

@flovilmart I added a fix for #4212
It should work on startup or if the indexes were created later on.

Also the indexes saved and returned from mongo are in the same format.
{ indexName: { indexColumn: value } }

flovilmart · 2017-11-25T19:55:31Z

Alright let's go with it!.

This reverts commit d5c4324.

* Add Indexes to Schema API * error handling * ci errors * postgres support * full text compound indexes * pg clean up * get indexes on startup * test compound index on startup * add default _id to index, full Text index on startup * lint * fix test

montymxb · 2017-11-26T00:10:31Z

@flovilmart something up here?

flovilmart · 2017-11-26T00:11:20Z

Ir’s Merged :)

* Add Indexes to Schema API * error handling * ci errors * postgres support * full text compound indexes * pg clean up * get indexes on startup * test compound index on startup * add default _id to index, full Text index on startup * lint * fix test

dplewis added 3 commits October 5, 2017 23:47

Add Indexes to Schema API

ad9998a

error handling

e544c91

ci errors

8341390

postgres support

af01977

vitaly-t reviewed Oct 6, 2017

View reviewed changes

full text compound indexes

86aea28

This was referenced Oct 8, 2017

Add Indexes via Schema parse-community/parse-php-sdk#357

Merged

Support for Parse Schema parse-community/Parse-SDK-JS#489

Merged

vitaly-t reviewed Oct 10, 2017

View reviewed changes

pg clean up

ec2c0cb

flovilmart changed the base branch from master to 3.x October 11, 2017 21:32

acinader approved these changes Oct 20, 2017

View reviewed changes

flovilmart force-pushed the 3.x branch from f6f1d9a to cd91e94 Compare October 21, 2017 15:48

montymxb reviewed Nov 15, 2017

View reviewed changes

flovilmart force-pushed the 3.x branch from 8fda61c to c36400a Compare November 16, 2017 01:29

test compound index on startup

bf055fd

montymxb approved these changes Nov 16, 2017

View reviewed changes

montymxb added the work in progress label Nov 21, 2017

dplewis added 2 commits November 21, 2017 17:56

add default _id to index, full Text index on startup

e251d44

lint

95fd385

montymxb reviewed Nov 22, 2017

View reviewed changes

fix test

102af94

dplewis requested a review from flovilmart November 23, 2017 16:57

flovilmart force-pushed the 3.x branch from c36400a to 4aa3788 Compare November 25, 2017 19:51

flovilmart approved these changes Nov 25, 2017

View reviewed changes

flovilmart merged commit d5c4324 into parse-community:3.x Nov 25, 2017

flovilmart added a commit that referenced this pull request Nov 25, 2017

Revert "Add Indexes to Schema API (#4240)"

ac120a8

This reverts commit d5c4324.

dplewis deleted the schema-index branch November 25, 2017 19:57

dplewis mentioned this pull request Jan 9, 2018

Newest Version Breaks Compound MongoDB Text Indices #4212

Closed

danepowell mentioned this pull request Jul 30, 2018

TypeError: this.adapter.updateSchemaWithIndexes is not a function benishak/parse-server-dynamodb-adapter#1

Open

snyk-bot mentioned this pull request Feb 25, 2022

refactor: security upgrade @graphql-tools/links from 8.2.1 to 8.2.2 #7823

Merged

Add Indexes to Schema API #4240

Add Indexes to Schema API #4240

Conversation

dplewis commented Oct 6, 2017 • edited Loading

codecov bot commented Oct 6, 2017 • edited Loading

Codecov Report

dplewis commented Oct 6, 2017

vitaly-t Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

vitaly-t Oct 6, 2017

Choose a reason for hiding this comment

vitaly-t Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

vitaly-t commented Oct 6, 2017

dplewis commented Oct 6, 2017

vitaly-t commented Oct 6, 2017

vitaly-t commented Oct 6, 2017

dplewis commented Oct 6, 2017

vitaly-t commented Oct 6, 2017

dplewis commented Oct 6, 2017 • edited Loading

flovilmart commented Oct 7, 2017

vitaly-t Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

vitaly-t Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

dplewis Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

vitaly-t Oct 11, 2017

Choose a reason for hiding this comment

vitaly-t commented Oct 11, 2017

acinader left a comment

Choose a reason for hiding this comment

flovilmart commented Oct 20, 2017

dplewis commented Oct 20, 2017

flovilmart commented Oct 20, 2017

dplewis commented Oct 20, 2017

flovilmart commented Oct 20, 2017

montymxb left a comment

Choose a reason for hiding this comment

dplewis commented Nov 16, 2017

montymxb commented Nov 16, 2017

montymxb left a comment

Choose a reason for hiding this comment

dplewis commented Nov 16, 2017

flovilmart commented Nov 16, 2017

montymxb commented Nov 21, 2017

montymxb Nov 22, 2017

Choose a reason for hiding this comment

montymxb Nov 22, 2017

Choose a reason for hiding this comment

dplewis commented Nov 22, 2017

flovilmart commented Nov 25, 2017

montymxb commented Nov 26, 2017

flovilmart commented Nov 26, 2017

dplewis commented Oct 6, 2017 •

edited

Loading

codecov bot commented Oct 6, 2017 •

edited

Loading

vitaly-t Oct 6, 2017 •

edited

Loading

vitaly-t Oct 6, 2017 •

edited

Loading

dplewis commented Oct 6, 2017 •

edited

Loading

vitaly-t Oct 10, 2017 •

edited

Loading

vitaly-t Oct 10, 2017 •

edited

Loading

dplewis Oct 11, 2017 •

edited

Loading