Add hash password to user

[lakhina]

@TimMoore I am not able to understand that what may be possible reason for https://travis-ci.org/lagom/online-auction-java/builds/249725258#L2272

When assertEquals(user, outcome.getReplies().get(0)); works perfectly fine then why there is null issue for hashpassword. I have checked with dbever that hashpassword have got properly saved and is not null in database.

[TimMoore]

@lakhina these failures are happening when the PersistentEntityTestDriver tests serialization of the CreatePUser command. It tries to ensure that if you serialize a command, then deserialize it again, the result is the same as the original input. This ensures that no data is lost during the serialization and deserialization process.

I still do not understand why this fails. Unfortunately, I cannot reproduce the error when running the test in IDEA, but I can reproduce it reliably when running with sbt, even if I just run the single test by itself.

I'll continue investigating why the behavior is different between IDEA and sbt.

[TimMoore]

This took me all day to debug, but I finally understand what's happening. It's a very complex interaction between Jackson, IDEA, sbt, and the way these classes are written. The passwordHash is being serialized correctly: it is during deserialization that it is lost.

The good news is that this found an actual bug (it's not just a problem with the test) and that it is simple to fix.

Explanation

When running in sbt

1. The JSON serialized from a CreatePUser command looks like this (pretty-printed):

   {
       "name" : "admin",
       "email" : "admin@gmail.com",
       "passwordHash" : "$2a$12$UFm7MA8nTs731iZLgPGYVe7MdsmAD0Lc4aylOjS/uMk6cG4Wf4W22"
   }

2. When deserializing back to a CreatePUser object, it looks for a constructor annotated with @JsonCreator, and finds CreatePUser(PUser user).
3. Because the constructor expects a PUser, Jackson then tries to deserialize the JSON into a PUser to pass to the CreatePUser constructor.
4. Because the PUser constructor declares @JsonProperty annotations on each of its parameters, Jackson tries to match these to the field names in the JSON.
5. This works OK for "name" and "email", but the "passwordHash" argument in the PUser constructor is declared like this:

   @JsonProperty("password") String passwordHash

   This means that it expects the field to be named "password" in the JSON document, instead of "passwordHash". Jackson does not find a match for "passwordHash", and so it passes null. This causes the test failure.

When running in IntelliJ IDEA

I was very confused about why I could not reproduce the failure in IDEA. Once I understood why it is failing in sbt, that helped me find the right place to look in the debugger to understand what's happening in IDEA.

I discovered that the deserializer for PUser is constructed differently when the test is running in IDEA, than when it is running in sbt. The PUser deserializer in IDEA is able to deserialize both JSON fields called password (taken from the @JsonProperty annotation) and passwordHash (taken from the private field name). It sees these as two separate fields, and can deserialize either one. Because the "passwordHash" name matches, it deserializes correctly.

So, why doesn't this work in sbt? Lagom configures Jackson with the optional parameter-names module. This takes advantage of a new feature in Java 8 that adds the names of constructor and method parameters to the bytecode, where you can call a reflection API to query the parameter names. With this module loaded, Jackson will use parameter names as an additional way to locate properties for deserialization. This means it matches the passwordHash parameter name in the PUser constructor with the passwordHash private field, and considers these linked. Then, because the constructor parameter has the @JsonProperty("password") annotation, it renames the passwordHash field to password for deserialization purposes.

So when running in sbt, rather than it finding two separate fields named passwordHash and password, it considers it to be just one field, which maps only to password. Since the field in the generated JSON is named passwordHash, this does not match.

Why is it different when running in sbt or IDEA? The parameter names feature in Java 8 is optional, and the parameter names are only included in the class file by the compiler if you run javac with the -parameters flag. In build.sbt we add the -parameters field to javacOptions in the commonSettings helper function. The code looks like this:

def commonSettings: Seq[Setting[_]] = eclipseSettings ++ Seq(
  javacOptions in Compile ++= Seq("-encoding", "UTF-8", "-source", "1.8"),
  javacOptions in(Compile, compile) ++= Seq("-Xlint:unchecked", "-Xlint:deprecation", "-parameters")
)

There is a bug in IntelliJ IDEA, which is that when you declare javacOptions in(Compile, compile), it does not see these settings, so it does not apply them to your project when it imports build.sbt. You can verify this by looking in "File | Settings | Build, Execution, Deployment | Compiler | Java Compiler" at the "Additional command line parameters" setting: it contains only "-encoding UTF-8", not "-Xlint:unchecked -Xlint:deprecation -parameters".

The reason that the compile options are set in two different scopes, is that there is a bug in sbt :weary: where javacOptions in Compile are also passed to the javadoc program. The second group of options are not valid for javadoc, so it fails when you run sbt doc.

However, javacOptions in compile (note the lowercase "c") works for both IDEA and sbt. The difference between Compile and compile, is that Compile refers to the sbt "configuration" (which applies to many tasks, including doc) and compile refers to the sbt "task" (which is only when the actual compiler is being run). Making this change (to set all options in javacOptions in Compile with a big "C") and refreshing the project in IDEA then causes it to pick up the -parameters option, and then the test failure is reproducible in IDEA, too.

(We can't commit that change, however, because it causes the doc task to fail as mentioned, and for some reason, the ConductR install task also calls doc implicitly, so it would break deployment, not just doc generation! I have tried several times over many months to find a magic combination of settings that works for both IDEA and sbt doc, but it seems to be a moving target that changes with each new release of the Scala plugin for IDEA. Right now, I can't find a setting that works consistently.)

phew!

Fix

After all of that complicated explanation, the fix is actually quite simple. In fact, there are multiple possible ways to fix it:

1. In PUser, change the @JsonProperty annotation for the passwordHash to "passwordHash", so that everything matches.
2. Remove all of the @JsonProperty annotations from PUser so that it relies on parameter name matching entirely.
3. Change the constructor of CreatePUser so that, instead of taking a PUser instance, it takes invidvidual name, email, and passwordHash parameters.

In fact, I think we should do both 2 and 3: change the constructors for both PUser and CreatePUser.

Furthermore, Lombok will generate constructors automatically for classes annotated with @Value, so you can even delete the constructors entirely, and everything just works. I think this is the best option: less code means less chance of mistakes. With both constructors deleted, UserEntityTest passes in both IDEA and sbt for me. We might as well delete the constructor in the PUserCreated event, as well.

@lakhina I hope all of this makes sense to you, but if not, all that really matters is the fix. Are you OK to make these changes?

[ignasi35]

Lombok will generate constructors automatically for classes annotated with @Value, so you can even delete the constructors entirely, and everything just works.

Even without a @JSonCreator annotated constructor? TIL! 👍

[TimMoore]

If there's only one constructor, Jackson will try to use it even without a @JsonCreator annotation.

[lakhina]

@TimMoore @ignasi35 Finally I am able to know the cause of failure. Thanks for helping. I will make the suggested changes asap.

[lakhina]

@TimMoore yayyyy. success was in to do less :wink:

[lakhina]

@TimMoore I feel it is mergeable now

[lakhina]

@TimMoore I have made the suggested changes. Please review