JanusGraph / janusgraph

JanusGraph: an open-source, distributed graph database
https://janusgraph.org
Other
5.3k stars 1.17k forks source link

replace all assert statements with Preconditions.checkState #770

Open amcp opened 6 years ago

amcp commented 6 years ago

I was bitten by an ugly bug in socialsensor/graphdb-benchmark because Titan/JG was failing silently when checking the validity of a manually-created vertex id. Actually throwing the exception will help us catch the poison pills and increase code quality.

amcp commented 6 years ago

there are 420 soft assertions in code:

$ grep -R "assert\ " . | grep src | wc
     420    2023   60383
porunov commented 5 years ago

@amcp Replacing all assertions in the code doesn't make much sense because most of them won't be triggered ever. By replacing assertions with Preconditions it means that we add new conditions in the production code. And some conditions are just not logical and should definitely be not triggered in the production code. My proposals are:

  1. Investigate each assertion and replace it with Preconditions only if it is wise (i.e. it can be triggered and we want to add a new condition). (Recommend)
  2. If you still want to replace all assertions then we can Investigate each assertion and replace some of them and remove assertions which are definitely won't be triggered.
farodin91 commented 5 years ago

I looked into the a stack overflow post about the assert statement: https://stackoverflow.com/questions/2758224/what-does-the-java-assert-keyword-do-and-when-should-it-be-used.

As there are lot of assert in the current code, I think should remove all not frontend(public interface) facing assertion checks and frontend facing checks should converted into preconditions or removed.

It seems assert statements are not very common and miss used or miss understood.