Skip to content

Add Indexes via Schema #357

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

Merged
merged 6 commits into from
Dec 2, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ Please note that this documentation contains the latest changes that may as of y
- [Version](#version)
- [Features](#features)
- [Schema](#schema)
- [Index](#index)
- [Purge](#purge)
- [Logs](#logs)
- [Contributing / Testing](#contributing--testing)
Expand Down Expand Up @@ -679,6 +680,19 @@ A schema can be removed via `delete`, but it must be empty first.
```php
$mySchema->delete();
```
#### 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

$schema->addString('field');
$index = [ 'field' => 1 ];
$schema->addIndex('index_name', $index);
$schema->save();

// Delete an index
$schema->deleteIndex('index_name');
$schema->save();
```

#### Purge
All objects can be purged from a schema (class) via `purge`. But be careful! This can be considered an irreversible action.
Expand Down
58 changes: 55 additions & 3 deletions src/Parse/ParseSchema.php
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,13 @@ class ParseSchema
*/
private $fields = [];

/**
* Indexes to create.
*
* @var array
*/
private $indexes = [];

/**
* Force to use master key in Schema Methods.
*
Expand Down Expand Up @@ -215,6 +222,10 @@ public function save()
$schema['fields'] = $this->fields;
}

if (!empty($this->indexes)) {
$schema['indexes'] = $this->indexes;
}

$result = ParseClient::_request(
'POST',
'schemas/'.$this->className,
Expand Down Expand Up @@ -247,14 +258,18 @@ public function update()
}

// Schema
$Schema['className'] = $this->className;
$Schema['fields'] = $this->fields;
$schema['className'] = $this->className;
$schema['fields'] = $this->fields;
$schema['indexes'] = $this->indexes;

$this->fields = [];
$this->indexes = [];

$result = ParseClient::_request(
'PUT',
'schemas/'.$this->className,
$sessionToken,
json_encode($Schema),
json_encode($schema),
$this->useMasterKey
);

Expand Down Expand Up @@ -348,6 +363,31 @@ public function addField($fieldName = null, $fieldType = 'String')
return $this;
}

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

Choose a reason for hiding this comment

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

Bit of a typo

Name of the index that will be created on Parse

* @param string $index Key / Value to be saved
*
* @throws \Exception
*
* @return ParseSchema indexes return self to create index on Parse
*/
public function addIndex($indexName = null, $index = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

{
if (!$indexName) {
throw new Exception('index name may not be null.', 105);
}

if (!$index) {
throw new Exception('index may not be null.', 105);
}

$this->indexes[$indexName] = $index;

return $this;
}

/**
* Adding String Field.
*
Expand Down Expand Up @@ -614,6 +654,18 @@ public function deleteField($fieldName = null)
];
}

/**
* Deleting a Index to Update on a Schema.
Copy link
Contributor

Choose a reason for hiding this comment

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

Deleting an Index

*
* @param string $indexName Name of the index will be deleted
Copy link
Contributor

Choose a reason for hiding this comment

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

that will be

*/
public function deleteIndex($indexName = null)
{
$this->indexes[$indexName] = [
'__op' => 'Delete',
];
}

/**
* Assert if ClassName has filled.
*
Expand Down
123 changes: 120 additions & 3 deletions tests/Parse/ParseSchemaTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,23 @@ public function testUpdateSchemaStream()
$this->assertNotNull($result['fields']['status']);
}

public function testUpdateMultipleSchemaStream()
{
ParseClient::setHttpClient(new ParseStreamHttpClient());

$schema = self::$schema;
$schema->save();

$schema->addString('name');
$schema->update();
$schema->update();
Copy link
Contributor

Choose a reason for hiding this comment

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

Double update?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, this is fine then.


$getSchema = new ParseSchema('SchemaTest');
$result = $getSchema->get();

$this->assertEquals(count($result['fields']), 5);
}

public function testUpdateSchemaCurl()
{
if (function_exists('curl_init')) {
Expand Down Expand Up @@ -411,7 +428,7 @@ public function testBadSchemaGet()
*/
public function testBadSchemaSave()
{
$this->setExpectedException('\Parse\ParseException');
$this->setExpectedException('\Exception');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@dplewis dplewis Dec 2, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

MacOS 10.12.6 Sierra

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for waiting. Now onto 1.4.0 👍


$user = new ParseUser();
$user->setUsername('schema-user');
Expand All @@ -427,7 +444,7 @@ public function testBadSchemaSave()
*/
public function testBadSchemaUpdate()
{
$this->setExpectedException('\Parse\ParseException');
$this->setExpectedException('\Exception');
Copy link
Contributor

Choose a reason for hiding this comment

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

same question for this here too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same above


$user = new ParseUser();
$user->setUsername('schema-user');
Expand All @@ -449,8 +466,108 @@ public function testBadSchemaDelete()
$user->setUsername('schema-user');
$user->setPassword('basicpassword');
$user->signUp();

$schema = new ParseSchema(self::$badClassName);
$schema->delete();
}

public function testCreateIndexSchemaStream()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

{
ParseClient::setHttpClient(new ParseStreamHttpClient());

$schema = self::$schema;
$schema->addString('name');
$index = [ 'name' => 1 ];
$schema->addIndex('test_index', $index);
$schema->save();

$getSchema = new ParseSchema('SchemaTest');
$result = $getSchema->get();
$this->assertNotNull($result['indexes']['test_index']);
}

public function testCreateIndexSchemaCurl()
{
if (function_exists('curl_init')) {
ParseClient::setHttpClient(new ParseCurlHttpClient());

$schema = self::$schema;
$schema->addString('name');
$index = [ 'name' => 1 ];
$schema->addIndex('test_index', $index);
$schema->save();

$getSchema = new ParseSchema('SchemaTest');
$result = $getSchema->get();
$this->assertNotNull($result['indexes']['test_index']);
}
}

public function testUpdateIndexSchemaStream()
{
ParseClient::setHttpClient(new ParseStreamHttpClient());

$schema = self::$schema;
$schema->save();
$schema->addString('name');
$index = [ 'name' => 1 ];
$schema->addIndex('test_index', $index);
$schema->update();

$getSchema = new ParseSchema('SchemaTest');
$result = $getSchema->get();
$this->assertNotNull($result['indexes']['test_index']);
}

public function testUpdateIndexSchemaCurl()
{
if (function_exists('curl_init')) {
ParseClient::setHttpClient(new ParseCurlHttpClient());

$schema = self::$schema;
$schema->save();
$schema->addString('name');
$index = [ 'name' => 1 ];
$schema->addIndex('test_index', $index);
$schema->update();

$getSchema = new ParseSchema('SchemaTest');
$result = $getSchema->get();
$this->assertNotNull($result['indexes']['test_index']);
}
}

public function testDeleteIndexSchema()
{
ParseClient::setHttpClient(new ParseStreamHttpClient());
Copy link
Contributor

Choose a reason for hiding this comment

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

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


$schema = self::$schema;
$schema->save();
$schema->addString('name');
$index = [ 'name' => 1 ];
$schema->addIndex('test_index', $index);
$schema->update();

$getSchema = new ParseSchema('SchemaTest');
$result = $getSchema->get();
$this->assertNotNull($result['indexes']['test_index']);

$schema->deleteIndex('test_index');
$schema->update();
$result = $getSchema->get();
$this->assertEquals(array_key_exists('text_index', $result['indexes']), false);
}

public function testIndexNameException()
{
$schema = self::$schema;
$this->setExpectedException('\Exception', 'index name may not be null.');
$schema->addIndex(null, null);
}

public function testIndexException()
{
$schema = self::$schema;
$this->setExpectedException('\Exception', 'index may not be null.');
$schema->addIndex('name', null);
}
}