Add Indexes via Schema #357

dplewis · 2017-10-08T20:38:26Z

Pending: parse-community/parse-server#4240

montymxb · 2017-10-11T19:11:48Z

Cool! Same deal here as with the aggregate queries. Once the server side functionality is in I'll go over this.

dplewis · 2017-11-26T00:14:59Z

@montymxb I added documentation and fixed the tests. The test weren't working locally at first until I updated my version of node from 6.10 to 9.2. I know the latest version of parse-server requires a minimum node version 6.11.4. Can you have a look into travis?

montymxb · 2017-11-26T00:39:27Z

Sure thing @dplewis , I'll see what's going on here...

montymxb · 2017-11-26T02:32:41Z

Took a look locally, seems to be just an outdated version of parse-server in cache. Dumped it and rerunning now. I did see one failure which looks to be consistent and is beyond this PR, probably due to a change in the server. We'll see if it reproduces when these run through.

dplewis · 2017-11-26T02:38:03Z

Are you talking about the testBadSchemaDelete or testCheckBadServer tests? I got those failing when running locally.

montymxb · 2017-11-26T02:41:44Z

Nope, schema tests look good actually. This is what I saw locally.

1) Parse\Test\ParseFileTest::testParseFileTypes
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'application/octet-stream'
+'null'

Curious if it's just a local issue or if it will repeat up here. If so I'll need to take a further look.

dplewis · 2017-11-26T02:42:23Z

@montymxb I got that as well

montymxb · 2017-11-26T03:09:23Z

Looks like there's still further issues starting up the test server. I'll have to look at this tomorrow when I have some time.

montymxb · 2017-11-29T10:23:10Z

@dplewis master is good now, you can merge against it when you have a moment.

codecov · 2017-11-29T13:34:08Z

Codecov Report

Merging #357 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #357      +/-   ##
==========================================
+ Coverage   99.01%   99.02%   +<.01%     
==========================================
  Files          38       38              
  Lines        3562     3579      +17     
==========================================
+ Hits         3527     3544      +17     
  Misses         35       35

Impacted Files	Coverage Δ
src/Parse/ParseSchema.php	`97.3% <100%> (+0.22%)`	⬆️

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 b8fdecc...c3531e7. Read the comment docs.

dplewis · 2017-11-29T13:47:58Z

@montymxb thanks for fixing the tests

montymxb · 2017-11-29T20:09:40Z

src/Parse/ParseSchema.php

+      /**
+     * Adding an Index to Create / Update a Schema.
+     *
+     * @param string $indexName Name of the index will created on Parse


Bit of a typo

Name of the index that will be created on Parse

montymxb · 2017-11-29T20:12:31Z

src/Parse/ParseSchema.php

+     *
+     * @return ParseSchema indexes return self to create index on Parse
+     */
+    public function addIndex($indexName = null, $index = null)


Not sure why default values of null are needed for both of the args. I think those defaults should be removed considering that a null value would cause this to throw an exception.

montymxb · 2017-11-29T20:14:13Z

src/Parse/ParseSchema.php

@@ -614,6 +654,18 @@ public function deleteField($fieldName = null)
        ];
    }

+    /**
+     * Deleting a Index to Update on a Schema.


Deleting an Index

montymxb · 2017-11-29T20:14:35Z

src/Parse/ParseSchema.php

+    /**
+     * Deleting a Index to Update on a Schema.
+     *
+     * @param string $indexName Name of the index will be deleted


that will be

montymxb · 2017-11-29T20:15:16Z

tests/Parse/ParseSchemaTest.php

+
+        $schema->addString('name');
+        $schema->update();
+        $schema->update();


Double update?

There was a bug where calling multiple updates would create multiple fields / indexes. I don't think this was intended functionality. Thus the testUpdateMultipleSchemaStream

Ahh, this is fine then.

montymxb · 2017-11-29T20:22:39Z

tests/Parse/ParseSchemaTest.php

        $schema = new ParseSchema(self::$badClassName);
        $schema->delete();
    }
+
+    public function testCreateIndexSchemaStream()


I would consolidate these pair tests into singular tests. You don't need to manually test each as part of the CI selectively runs the entire test suite against the stream client in addition to the curl client. The other reason being if we ever add additional clients in the future we would have to add additional tests if we did it this way, versus just adding another CI run selectively using the alternate client instead.

There are some tests in here that still do this and it was probably me who added them. Those were early tests before I had setup the independent curl/stream CI runs. Honestly they can be selectively phased out at now. But to sum it up we shouldn't add additional stream differing tests (unless there's an extenuating circumstance) as it's redundant now.

Just to detail where it is you can see the stream client testing in the matrix for 5.4 and HHVM and later you can see how it is actually factored into the tests here.

montymxb · 2017-11-29T20:23:40Z

tests/Parse/ParseSchemaTest.php

+
+    public function testDeleteIndexSchema()
+    {
+        ParseClient::setHttpClient(new ParseStreamHttpClient());


Since this one is just on it's own you can just clear out the manual client.

montymxb · 2017-11-29T20:25:05Z

README.md

+#### Index
+Indexes support efficient execution of queries from the database. MasterKey is required.
+```php
+// To add an index, the field must exist before you create an index


Could you add a few lines showing how to check existing indexes?

montymxb

Alright tests are finally good now. I gave this a thorough review, typos, suggestions, etc. If you can make those changes I'll give this another go and we should be pretty close to good here.

montymxb · 2017-12-02T00:53:47Z

tests/Parse/ParseSchemaTest.php

@@ -411,7 +429,7 @@ public function testBadSchemaGet()
     */
    public function testBadSchemaSave()
    {
-        $this->setExpectedException('\Parse\ParseException');
+        $this->setExpectedException('\Exception');


One last thing. Is there a particular reason you changed the expected exception here?

https://github.com/dplewis/parse-php-sdk/blob/schema-index/src/Parse/ParseSchema.php#L277

It throws exception instead of parse exception. The test were failing locally for some reason.

On a side note testBadSchemaDelete doesn't pass locally for me. If I remove the spaces from $badClassName = "<Bad~ Class~ Name> it passes. Weird.

Ahh that's something I missed. That change is good then. And quick question, are you running these tests on a windows machine by chance?

MacOS 10.12.6 Sierra

Hmm, good on mine and we're on the same setup. There's certainly a reason for it but I wonder what 🤔 . Anyways this is all good then.

Thanks for waiting. Now onto 1.4.0 👍

montymxb · 2017-12-02T00:54:01Z

tests/Parse/ParseSchemaTest.php

@@ -427,7 +445,7 @@ public function testBadSchemaSave()
     */
    public function testBadSchemaUpdate()
    {
-        $this->setExpectedException('\Parse\ParseException');
+        $this->setExpectedException('\Exception');


same question for this here too.

montymxb

Everything looks good. Just need to hear about a change on a pair of expected exceptions and we're good.

montymxb

approved

montymxb · 2017-12-02T01:51:45Z

Sweet we're ready for 1.4.0. I'll craft a release later tonight.

Add Indexes via Schema

56a3fc0

support for multiple update

ec14e63

montymxb added this to the 1.4.0 milestone Nov 13, 2017

dplewis added 2 commits November 25, 2017 15:55

Merge branch 'master' into schema-index

e9be5d7

Documentation and Test Fix

a921116

Merge branch 'master' into schema-index

f3ff221

montymxb reviewed Nov 29, 2017

View reviewed changes

montymxb suggested changes Nov 29, 2017

View reviewed changes

Typos and test updates

c3531e7

montymxb reviewed Dec 2, 2017

View reviewed changes

montymxb approved these changes Dec 2, 2017

View reviewed changes

montymxb merged commit e0e2dc8 into parse-community:master Dec 2, 2017

dplewis deleted the schema-index branch May 22, 2018 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Indexes via Schema #357

Add Indexes via Schema #357

dplewis commented Oct 8, 2017

montymxb commented Oct 11, 2017

dplewis commented Nov 26, 2017 •

edited

Loading

montymxb commented Nov 26, 2017

montymxb commented Nov 26, 2017

dplewis commented Nov 26, 2017 •

edited

Loading

montymxb commented Nov 26, 2017

dplewis commented Nov 26, 2017

montymxb commented Nov 26, 2017

montymxb commented Nov 29, 2017

codecov bot commented Nov 29, 2017 •

edited

Loading

dplewis commented Nov 29, 2017

montymxb Nov 29, 2017

montymxb Nov 29, 2017

montymxb Nov 29, 2017

montymxb Nov 29, 2017

montymxb Nov 29, 2017

dplewis Nov 29, 2017

montymxb Nov 29, 2017

montymxb Nov 29, 2017

montymxb Nov 29, 2017

montymxb Nov 29, 2017

montymxb Nov 29, 2017

montymxb left a comment

montymxb Dec 2, 2017

dplewis Dec 2, 2017 •

edited

Loading

montymxb Dec 2, 2017

dplewis Dec 2, 2017

montymxb Dec 2, 2017

dplewis Dec 2, 2017

montymxb Dec 2, 2017

dplewis Dec 2, 2017

montymxb left a comment

montymxb left a comment

montymxb commented Dec 2, 2017

Add Indexes via Schema #357

Add Indexes via Schema #357

Conversation

dplewis commented Oct 8, 2017

montymxb commented Oct 11, 2017

dplewis commented Nov 26, 2017 • edited Loading

montymxb commented Nov 26, 2017

montymxb commented Nov 26, 2017

dplewis commented Nov 26, 2017 • edited Loading

montymxb commented Nov 26, 2017

dplewis commented Nov 26, 2017

montymxb commented Nov 26, 2017

montymxb commented Nov 29, 2017

codecov bot commented Nov 29, 2017 • edited Loading

Codecov Report

dplewis commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montymxb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dplewis Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montymxb left a comment

Choose a reason for hiding this comment

montymxb left a comment

Choose a reason for hiding this comment

montymxb commented Dec 2, 2017

dplewis commented Nov 26, 2017 •

edited

Loading

dplewis commented Nov 26, 2017 •

edited

Loading

codecov bot commented Nov 29, 2017 •

edited

Loading

dplewis Dec 2, 2017 •

edited

Loading