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

Configuration validation exceptions must tell validation rules/values #67

Closed
zdenek-jonas opened this issue Feb 3, 2020 · 9 comments
Closed
Assignees

Comments

@zdenek-jonas
Copy link
Contributor

Our Documentation in page:
https://manual.docs.microstream.one/data-store/configuration/properties
says:
Minimum file size in bytes of a storage file to avoid merging with other files during housekeeping.
Must be greater than 1, maximum is 2GB.

It should be there, which format should be used.

When i write: 500
I received:
one.microstream.storage.exceptions.StorageExceptionInvalidConfiguration: Invalid file size: 500
Why is this number invalid?

When i write 10Mb
I received:
java.lang.IllegalArgumentException: Nonsensical size limits: min file size = 10000000, max file size = 8388608
I would write: Min size limit cannot be higher as actual max size limit. This number has many '0', its hard to read it.

Ok, I write: 20000000
I received:
one.microstream.storage.exceptions.StorageExceptionInvalidConfiguration: Invalid file size: 20000000
Why is this number invalid?

I write: 5kb
dann passt.
(previous edit was from ZJ via my computer during a meeting)

@tm-ms tm-ms changed the title Documentation - dataFileMinSize Configuration validation exceptions must tell validation rules/values Feb 3, 2020
@tm-ms tm-ms self-assigned this Feb 3, 2020
@tm-ms
Copy link
Contributor

tm-ms commented Feb 17, 2020

This problem is split over two layers/projects: Core and EmbeddedConfiguration.

The "Nonsensical ..." point was a problem in the core project.
I have overhauled all component types of StorageConfiguration to perform proper validation with meaningful exception messages.

More specifically, the following types now have "Validation" sub-type using a unified pattern for validating parameters.
StorageChannelCountProvider
StorageHousekeepingController
StorageDataFileEvaluator
StorageEntityCacheEvaluator

The following types just check for non-null values, so I left them with simple notNull(...) checks:
StorageFileProvider
StorageBackupSetup

See next comment for the second aspect.

@tm-ms
Copy link
Contributor

tm-ms commented Feb 17, 2020

The other problems (with "Invalid file size") are located in the EmbeddedConfiguration project.
They are also technically wrong as far as I can see. The file bounds can have arbitrary values inside the minimum and maximum limit.
This has to be fixed there.
In order to prevent redundant validation code, the "Validation" sub types' methods described above can/should be used.

@tm-ms tm-ms assigned fh-ms and unassigned tm-ms Feb 17, 2020
@fh-ms
Copy link
Contributor

fh-ms commented Feb 17, 2020

Adapted validators in EmbeddedConfiguration project.

@fh-ms
Copy link
Contributor

fh-ms commented Feb 17, 2020

Ok, I write: 20000000
I received:
one.microstream.storage.exceptions.StorageExceptionInvalidConfiguration: Invalid file size: 20000000
Why is this number invalid?

File sizes without suffix are now interpreted as bytes.

@zdenek-jonas
Copy link
Contributor Author

zdenek-jonas commented Feb 21, 2020

Retest:
There are still something.
I just define the smallestSizeOfDataFile setDataFileMinimumSize:
0: correct:

Specified file minimum size of 0 is not in the valid range of [1024, 2147483647].

2147483647: a little nonsense, the maximum size is given in negative numbers, and I did not specify it.

java.lang.IllegalArgumentException: Specified file maximum size of -2147483648 is smaller than the specified file minimum size of 2147483647.

@tm-ms
Copy link
Contributor

tm-ms commented Feb 21, 2020

There's a copy&paste error in the exception for the maximum size check:

// can technically never happen for now, but the max value might change
if(fileMaximumSize >= maximumFileSize())
{
	 // (17.02.2020 TM)EXCP: proper exception
	throw new IllegalArgumentException(
		"Specified file maximum size of " + fileMinimumSize
		+ " is not in the valid range of ["
		+ minimumFileSize() + ", " + maximumFileSize() + "]."
	);
}

However, since fileMaximumSize is an int and maximumFileSize() is hard-coded to Integer.MAX_VALUE, I fail to see how that exception can ever happen. Hence the comment.
No idea how fileMinimumSize could have become negative except if that value was passed by the user.

@fh-ms
Copy link
Contributor

fh-ms commented Feb 21, 2020

Fixed copy&paste error, and also parameter mixup in configuration layer.

@tm-ms
Copy link
Contributor

tm-ms commented Feb 21, 2020

Okay, I was confused: The exception message was about the "smaller" case, not the case I pasted the code snippet for.
Nevertheless, 2 points of stupidity on my part in the pasted code:
1.) fileMinimumSize must still be fileMaximumSize
2.) ">=" is wrong. It must be ">".

edit: Ah, FH already fixed my stupidity before my comment.

@zdenek-jonas
Copy link
Contributor Author

Ok, test successful.

@fh-ms fh-ms transferred this issue from another repository Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants