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

Value validation for indexed properties. #8404

Merged
merged 3 commits into from Nov 25, 2016

Conversation

Projects
None yet
4 participants
@MishaDemianenko
Contributor

MishaDemianenko commented Nov 18, 2016

Introduce validation of property values for legacy and schema indexes.
Create validators for empty/null/allowable cases. Perform validation during property updated and index population.

changelog: Add validation of schema and legacy index values: do not allow nulls, values that over exceed lucene max term size, etc.

}
if ( value instanceof String )
{
IndexValueLengthValidator.INSTANCE.validate( ((String) value).getBytes() );

This comment has been minimized.

@lutovich

lutovich Nov 21, 2016

Contributor

getBytes() here and in IndexValueValidator will cause decoding of chars and copying them in a newly allocated byte array. is this ok performance wise? maybe we can calculate bytes length manually?

@lutovich

lutovich Nov 21, 2016

Contributor

getBytes() here and in IndexValueValidator will cause decoding of chars and copying them in a newly allocated byte array. is this ok performance wise? maybe we can calculate bytes length manually?

This comment has been minimized.

@MishaDemianenko

MishaDemianenko Nov 21, 2016

Contributor

you are obviously right, those guys where just automagically moved from LuceneDocumentStructure to a specific validator, maybe it's time to clean that as well even if that is not the goal of this pr...

@MishaDemianenko

MishaDemianenko Nov 21, 2016

Contributor

you are obviously right, those guys where just automagically moved from LuceneDocumentStructure to a specific validator, maybe it's time to clean that as well even if that is not the goal of this pr...

This comment has been minimized.

@tinwelint

tinwelint Nov 22, 2016

Member

So schema indexes already did this, right? Then it's "OK" to add this slight extra garbage to legacy indexes as well imho

@tinwelint

tinwelint Nov 22, 2016

Member

So schema indexes already did this, right? Then it's "OK" to add this slight extra garbage to legacy indexes as well imho

This comment has been minimized.

@lutovich

lutovich Nov 22, 2016

Contributor

Never mind me. This validation has already been present for schema indexes just deeper in the call stack.

@lutovich

lutovich Nov 22, 2016

Contributor

Never mind me. This validation has already been present for schema indexes just deeper in the call stack.

This comment has been minimized.

@MishaDemianenko

MishaDemianenko Nov 22, 2016

Contributor

ok, that was the case for schema indexes before and we definitely can take a look into that in future if that will be required, i think for now it should be fine

@MishaDemianenko

MishaDemianenko Nov 22, 2016

Contributor

ok, that was the case for schema indexes before and we definitely can take a look into that in future if that will be required, i think for now it should be fine

Show outdated Hide outdated .../src/main/java/org/neo4j/index/impl/lucene/legacy/LuceneLegacyIndex.java
@@ -131,16 +132,9 @@ private static void assertValidKey( String key )
}
}
private static void assertValidValue( Object singleValue )
protected void assertValidValue( Object value )

This comment has been minimized.

@tinwelint

tinwelint Nov 22, 2016

Member

Looks like this methods isn't actually needed anymore, or it could at least still remain private static, couldn't it?

@tinwelint

tinwelint Nov 22, 2016

Member

Looks like this methods isn't actually needed anymore, or it could at least still remain private static, couldn't it?

This comment has been minimized.

@MishaDemianenko

MishaDemianenko Nov 22, 2016

Contributor

yeah that was sneaked experimentation, cleaned up

@MishaDemianenko

MishaDemianenko Nov 22, 2016

Contributor

yeah that was sneaked experimentation, cleaned up

@tinwelint

This comment has been minimized.

Show comment
Hide comment
@tinwelint

tinwelint Nov 22, 2016

Member

@MishaDemianenko looks like it needs rebase

Member

tinwelint commented Nov 22, 2016

@MishaDemianenko looks like it needs rebase

@tinwelint

This comment has been minimized.

Show comment
Hide comment
@tinwelint

tinwelint Nov 22, 2016

Member

In general I think this looks OK

Member

tinwelint commented Nov 22, 2016

In general I think this looks OK

@MishaDemianenko

This comment has been minimized.

Show comment
Hide comment
@MishaDemianenko

MishaDemianenko Nov 22, 2016

Contributor

@tinwelint cleaned up and rebased

Contributor

MishaDemianenko commented Nov 22, 2016

@tinwelint cleaned up and rebased

@burqen

Small comments on test coverage. Looks good otherwise.

getValidator().validate( RandomStringUtils.randomAlphabetic( 5 ) );
getValidator().validate( RandomStringUtils.randomAlphabetic( 10 ) );
getValidator().validate( RandomStringUtils.randomAlphabetic( 250 ) );
getValidator().validate( RandomStringUtils.randomAlphabetic( 450 ) );

This comment has been minimized.

@burqen

burqen Nov 22, 2016

Contributor

Would be nice with a test for exactly MAX_TERM_LENGTH as well.

@burqen

burqen Nov 22, 2016

Contributor

Would be nice with a test for exactly MAX_TERM_LENGTH as well.

INSTANCE.validate( RandomUtils.nextBytes( 30 ) );
INSTANCE.validate( RandomUtils.nextBytes( 300 ) );
INSTANCE.validate( RandomUtils.nextBytes( 4303 ) );
INSTANCE.validate( RandomUtils.nextBytes( 13234 ) );

This comment has been minimized.

@burqen

burqen Nov 22, 2016

Contributor

Test array of MAX_TERM_LENGTH here as well.

@burqen

burqen Nov 22, 2016

Contributor

Test array of MAX_TERM_LENGTH here as well.

getValidator().validate( RandomUtils.nextBytes( 3 ) );
getValidator().validate( RandomUtils.nextBytes( 30 ) );
getValidator().validate( RandomUtils.nextBytes( 450 ) );
getValidator().validate( RandomUtils.nextBytes( 4556 ) );

This comment has been minimized.

@burqen

burqen Nov 22, 2016

Contributor

Again, MAX_TERM_LENGTH

@burqen

burqen Nov 22, 2016

Contributor

Again, MAX_TERM_LENGTH

@burqen

This comment has been minimized.

Show comment
Hide comment
@burqen

burqen Nov 22, 2016

Contributor

@MishaDemianenko This would be a candidate for changelog 👍

Contributor

burqen commented Nov 22, 2016

@MishaDemianenko This would be a candidate for changelog 👍

@MishaDemianenko

This comment has been minimized.

Show comment
Hide comment
@MishaDemianenko

MishaDemianenko Nov 22, 2016

Contributor

@burqen additional tests added for all things except IndexValueValidatorTest since you can directly ask for those number of bytes for array since those will be encoded by array encoder. We can pretend and guess border line but i do not think we need to do that.
And change log label added and message provided

Contributor

MishaDemianenko commented Nov 22, 2016

@burqen additional tests added for all things except IndexValueValidatorTest since you can directly ask for those number of bytes for array since those will be encoded by array encoder. We can pretend and guess border line but i do not think we need to do that.
And change log label added and message provided

@burqen

This comment has been minimized.

Show comment
Hide comment
@burqen

burqen Nov 22, 2016

Contributor

@MishaDemianenko Looks good! Approved by me once green.

Contributor

burqen commented Nov 22, 2016

@MishaDemianenko Looks good! Approved by me once green.

@burqen

burqen approved these changes Nov 22, 2016

@burqen

This comment has been minimized.

Show comment
Hide comment
@burqen

burqen Nov 23, 2016

Contributor

@MishaDemianenko triggered another run for Linux build.
The muted test that failed, is that not related?

Contributor

burqen commented Nov 23, 2016

@MishaDemianenko triggered another run for Linux build.
The muted test that failed, is that not related?

MishaDemianenko added some commits Nov 16, 2016

Value validation for indexed properties.
Introduce validation of property values for legacy and schema indexes.
Create validators for empty/null/allowable cases. Perform validation during property updated and index population.
@MishaDemianenko

This comment has been minimized.

Show comment
Hide comment
@MishaDemianenko

MishaDemianenko Nov 23, 2016

Contributor

@burqen ok, rebased and old failure gone now it's failing with new 3.2 non related failures

Contributor

MishaDemianenko commented Nov 23, 2016

@burqen ok, rebased and old failure gone now it's failing with new 3.2 non related failures

@burqen burqen merged commit cbcc0a3 into neo4j:3.2 Nov 25, 2016

2 of 3 checks passed

Run Linux Tests (pull-requests) TeamCity build failed
Details
Run Tools Compile (pull-requests) TeamCity build finished
Details
Whitelist and License check (pull-requests) TeamCity build finished
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment