Concept API clean up, refactors, and bug fixes

[haikalpribadi]

What is the goal of this PR?

Following the recent PR by @alexjpwalker (#157) which significantly changed the way Client Java has been implemented to work with Grakn Core 2.0, I've noticed there were lots of architectural and design issues that needed to be addressed before we proceed with completing the rest of the functionalities. However, this PR contains revision of the codebase that was not only written in the last PR. This PR revises the codebase that includes PRs from @adammitchelldev and some from @flyingsilverfin. The changes in this PR is important to take note for future reference during development.

What are the changes implemented in this PR?

• Concept.asRemote(...) should not take take in Concepts, this is not as clear to the user as simpy expecting a Transaction in which the user is already familiar with. We should keep the terms that the users need to work with as few as possible
• Concepts did not need to be split into interface and implementation. It's okay for the //concept package to be depending on the Grakn.Transaction interface as it is a the API interfaces, in which all packages necessarily depend on.
• We should not have static methods in interface classes to construct objects of the implementation classes. We do not want to have references to implentation classes inside interfaces. Additionally, and importantly, the interfaces in client-java will be extremely user facing and one of it's main purpose is ease-of-understanding and learnability of the API by the user. We don't want to pollute and overload the API for the user to understand.
• The static constructor methods should also return the object as the implementation class rather than the interface, so that when an implementation class (e.g. AttributeImpl) calls a constructor method on another implementation class (e.g. Thingimpl) then the first class has access to the all the implementation methods of the second class.
• There were places where exception messages only needed Class.getSimpleName() and not Class.getCanonicalName()
• There were lots of source code that had AGPL license as they were copied from other repos. Client Java needs to have Apache license.
• We should try hard to never do manual casting of objects whenever we can, especially when downcasting methods are provided, such as ThingType.asEntityType().
• All methods that return concepts should always return Local concepts by default, and users must always explicity convert them to Remote first before being able to do any "remote concept operations". Otherwise, the user will not be excplicitly aware that they're doing operations over the network - which is the goal of us introducing Local vs Remote concepts in the first place. Thus, we also need not to provide the asRemote() method on at super Concept interface which covers both Local and Remote concepts. We only (and should only) need to provide the asRemote() method on Local concepts.
• Local concepts did not have their equals() and hash() function defined.
• It seems like we lost the feature of calling getType() on Thing.Local concept in the previous PR (which is okay). However, we left the ThingImpl.Local constructor commented out and throws an exception. We should implement Thing.Local properly without getType() for now, and work on bringing it back properly in a subsequent PR.
• RoleType was missing equals() and hashCode() function even though we know their identity depends on their scopedLabel
• RoleTypes were serialised in GRPC with their label and scopedLabel, which is not the correct canonical representation of a RoleType. We should just store label and scope, which would be the correct canonical representation. This would allow us to implement equality and hashing function for Types a lot easier.
• Since now we're parsing ConceptProto in the Concept classes, we should at least be neat and consistent about it. Local concepts were passing it entire ConceptProto objects, and parsing the values inside the constructor, but Remote concepts were parsing the values in the static constructor methods. We should stick with one style.
• We weren't passing scope or scopedLabel in ConceptMethod.Type.Req and ConceptMethod.Type.Req which means that concept methods called from RoleTypes would fail.
• RoleType.Remote was missing getSupertype() and was mistakenly provided the method setSupertype(...)
• TypeImpl.Remote implemented getSupertypes() and getSubtypes() but not getSupertype and instead provided getSupertypeInternal(constructorFn)and setSupertypeInternal(constructorFn). First, we don't need getSupertypes() and getSubtypes() at TypeImpl.Remote. Second, we should be consistent and provide the same style API - this should have been the code smell.
• default case in a switch should come last in the order of cases.
• There were lots of code that could have been shortened / compacted in to much fewer lines; by either doing static imports to nested classes/methods, and inlining variables and methods. Let's make this a habit to reduce the amount of code to digest.
• asRemote(tx) methods of Remote concepts should not simply return itself (this) without considering the transaction argument. What if the user passes a new transaction object? If we don't expect this, then we should throw an exception, but we shouldn't simply ignore the argument. I think we should create a new remote concept object with the new transaction.

Additionally:

• Removed all Javadocs comments so we can rewrite them properly
• Bumped the version to 2.0.0

TODO (to follow up on after this PR):

• ErrorMessages were variable-ised such that error messages with similar syntax but came from different sources would use the same ErrorMessage instance. For example "'%s' can not be null or empty." with code CIN2 was used for missing IIDs, labels, and database names. The problem with this approach is that now error codes no longer uniquely identify the problem/bug that caused the exception, which was the point of our new ErrorMessage architecture utilising error codes.
• Default methods that override another interface method, such as ThingType.Local.asThingType() and ThingType.Remote.asThingType() should simply implemented in the implementation class, rather than implemented as a default method in the interface. This way we can minimise the number of methods to document on the interfaces.
• Let's take EntityType: the EntityType.Remote interface already extends ThingType.Remote and EntityType. Which means it does not depend on EntityType.Local at all. When users query for a concept the first time, they always get EntityType.Local concept, but would naturally be simply assigned to EntityType as users won't expect to declare their variables as EntityType.Local. And given that we want allow the user to get the "stored" / "local" fields immediately, we declare these fields at the parent interface (right now this only applies to Attribute which provides getValue(). The fact that EntityType.Remote extends EntityType also allows us to retrieve any "stored"/"local" fields even from a Remote concept. But given all the above, we can see that there's no need to have a Local sub interface for every concept type. We can simply treat the main interface, such as EntityType in this case, to be the "local" concept, and when we call asRemote(tx) it converts to EntityType.Remote concept, which would be extending EntityType.
• Should we consider removing Grakn.client(...) builder method on the main interface? We already know that it's not ideal that the interface depends on the implementation (grakn.core.client.rpc.RPCClient) to achieve this, but now we also found out that this restricts our users to have to be using Java 8 and higher. I think there's a strong case now to keep the UX to be: Grakn.Client client = new GraknClient(...); where the user also has to import grakn.core.client.rpc.GraknClient.

[flyingsilverfin]

All sounds good to me!

[alexjpwalker]

Reviewed and left some comments.

[alexjpwalker]

We have a guard in checkstyle, defined in graknlabs_dependencies, that checks every .java file and validates that the license is correct. If it is wrong (say, the following text is not present on line 2: Licensed to the Apache Software Foundation), then the build fails.

Unfortunately, it does not run on our BUILD files. We should fix this in graknlabs_dependencies.

[haikalpribadi]

That would be nice, yes. But I don't know how we could do that? 🤔

[alexjpwalker]

Also we should kill GetValue from Protocol and always return the cached value on the local instance!

[haikalpribadi]

I think there's a strong case now to keep the UX to be: Grakn.Client client = new GraknClient(...); where the user also has to import grakn.core.client.rpc.GraknClient.

This should be fine provided we do actually rename RPCClient to GraknClient.

Yes, that's what we want.

This would make the class name inconsistent with the rest of the package, but I think that's fine.

Yes, that's what we expect.

[haikalpribadi]

Also we should kill GetValue from Protocol and always return the cached value on the local instance!

Yes, I agree!