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

util: Add additional custom field apis to serach by objectType, field… #1109

Merged
merged 3 commits into from Mar 1, 2019

Conversation

Projects
None yet
2 participants
@sbrossie
Copy link
Member

sbrossie commented Feb 28, 2019

…Name (and possibly value)

@pierre
Copy link
Member

pierre left a comment

Just one thing to double check regarding the SQL.

For the endpoint, /search?fieldName=&fieldValue=&objectType= maybe?

where
object_type = :objectType
and field_name = :fieldName
<if(fieldValue)>and field_value = '<fieldValue>'<endif>

This comment has been minimized.

@pierre

pierre Feb 28, 2019

Member

To be verified, but I believe @Defined parameters are passed as-is, without sanitation (unlike @Bind parameters). This could potentially open the door to SQL injections (e.g. fieldValue = ' limit 0; truncate accounts;) or limit search capabilities (e.g. field values with special characters)?

This comment has been minimized.

@sbrossie

sbrossie Feb 28, 2019

Author Member

Fixed is as we discussed. See e08d6a4


@Override
public Pagination<CustomFieldModelDao> searchCustomFields(final String fieldName, final String fieldValue, final ObjectType objectType, final Long offset, final Long limit, final InternalTenantContext context) {
return null;

This comment has been minimized.

@pierre

pierre Feb 28, 2019

Member

Symmetry (throw)?

This comment has been minimized.

@sbrossie

sbrossie Feb 28, 2019

Author Member

Fixed in e08d6a4

final String NON_SEARCHABLE_FILED_NAME = "NotToBeFound";

final List<CustomFieldModelDao> input = ImmutableList.of(
new CustomFieldModelDao(internalCallContext.getCreatedDate(), SEARCHABLE_FILED_NAME, "The world will collapse soon!", someUUId, ObjectType.ACCOUNT),

This comment has been minimized.

@pierre

This comment has been minimized.

@sbrossie

sbrossie Feb 28, 2019

Author Member

A little note of optimism... 😆

count(1) as count
from <tableName()>
where
object_type = :objectType

This comment has been minimized.

@pierre

pierre Feb 28, 2019

Member

Do we need new indices or are we all set?

This comment has been minimized.

@sbrossie

sbrossie Feb 28, 2019

Author Member

With our current indexes, we are relying on the existing custom_fields_tenant_account_record_id:

+----+-------------+---------------+------------+------+-----------------------------------------------------+----------------------------------------+---------+-------+------+----------+----------------------------------------------------+
| id | select_type | table         | partitions | type | possible_keys                                       | key                                    | key_len | ref   | rows | filtered | Extra                                              |
+----+-------------+---------------+------------+------+-----------------------------------------------------+----------------------------------------+---------+-------+------+----------+----------------------------------------------------+
|  1 | SIMPLE      | custom_fields | NULL       | ref  | custom_fields_tenant_account_record_id,my_new_index | custom_fields_tenant_account_record_id | 8       | const |    1 |    50.00 | Using index condition; Using where; Using filesort |
+----+-------------+---------------+------------+------+-----------------------------------------------------+----------------------------------------+---------+-------+------+----------+----------------------------------------------------+

I tried the following indexes:

  • Composite (tenant_record_id, field_name)
  • Composite (field_name, tenant_record_id)
  • Simple (field_name)

None of those get used and mysql keeps relying on existing (tenant_record_id, account_record_id);, which is not ideal...

If i force the index, then i see mysql using it, but it worth it -- and how portable is this for other engines?

This comment has been minimized.

@pierre

pierre Mar 1, 2019

Member

Wondering if it's because you don't have enough data in the table (and any one of them is good enough)?

Let's keep an eye on it in production and adjust as data grows.

This comment has been minimized.

@sbrossie

sbrossie Mar 1, 2019

Author Member

Yep, agreed... we could retry on G. DB as well to see what we see...

sbrossie added some commits Feb 28, 2019

@pierre

pierre approved these changes Mar 1, 2019

@sbrossie sbrossie merged commit e14e49a into work-for-release-0.21.x Mar 1, 2019

5 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: integration-tests Your tests passed on CircleCI!
Details
ci/circleci: test-h2 Your tests passed on CircleCI!
Details
ci/circleci: test-mysql Your tests passed on CircleCI!
Details
ci/circleci: test-postgresql Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.