Generator-jhipster: Boolean type when required should be primitive type and getter should start with get when it's not

[yelhouti]

Overview of the issue

When attribute type is boolean and is required the java bean intrespector doesn't see the getter that starts with "is" exampe isMyBoolean. check here for source code. this creates problem with validatores when they encouter a @NotNull Boolean myVar is should be boolean instead to avoid NullPointerExcpetions, it also avoids problems when persising data...
For Non required field a getter should be used for java to see it as linked to the bean getMyBoolean same code shows why.

When using required boolean, the validator crashed with 500 (internal server error) instead of 400 "Bad Request".

Reproduce the error

generate service with a JDL with required boolean (and DTOs enabled just to make sure) and try to insert with null boolean.

Related issues

Suggest a Fix

Suggested above

JHipster Version(s)

4.14.x but it pretty sure it's still here since I didn't find an issue/commit that fixes it.

JHipster configuration

Entity configuration(s) entityName.json files generated in the .jhipster directory

entity Document {
    employeeUploaderId String
    type String required
    visibleToCustomer Boolean required
    description String required
    filename String required
    size Integer required
    createdAt ZonedDateTime required
    viewedAt ZonedDateTime
}

Browsers and Operating System

• [x] Checking this box is mandatory (this is just to show you read everything)

[jdubois]

Yes this should affect all versions, but I'm sorry I'm not sure of what you mean.

If my boolean is called "test", then the method should be called "isTest", is that correct for you?

Also, I'm not sure a "null" boolean can exist, it can only be true or false, and if you use a Boolean instance that is null, then for me it is false.

There are definitely things to check here, maybe you could create a small unit test so we can all see how this work, and be sure it never happens again.

[cbornet]

isXXX can only be used for the primitive boolean, not for the wrapper Boolean.

[jdubois]

Oh I need to learn better how unboxing work

[einar-saukas]

Also, I'm not sure a "null" boolean can exist, it can only be true or false, and if you use a Boolean instance that is null, then for me it is false.

It's not the same thing. A Boolean storing "null" means "undefined", it usually indicates that the actual value is not known... yet.

In a perfect world, all knowledge would be exact. Field "isActive" in a table about companies can only be true or false. Field "isAlive" in a table about people can only be true or false. Field "isOpen" in a table about doors can only be true or false. And son on.

In practice, it doesn't always work. Sometimes your business requirements require storing records, even when you don't have a complete information. Sometimes you need to import new records from a third-party source that simply doesn't have some of this information. And so on. In those cases, it may be necessary to flag the missing information as unknown (thus storing "null") instead of guessing either true or false. It's the same logic that applies to other datatypes, not just Boolean.

Certain developers adopt the philosophy that a Boolean should never be "null", that's fine! They can always declare every Boolean as REQUIRED in JDL and NOT NULL in the database. Everything will work as expected here.

However other developers consider it sometimes necessary to use "null" in certain fields (including Boolean) in real life cases. They will define this field as NULL in the database, non-required in JDL, and expect JHipster to behave accordingly.

As a generic framework, JHipster should be flexible enough to support both options, without "taking sides". In particular, I believe the lack of proper support for non-required boolean described in #8777 is quite relevant.

[yelhouti]

I agree with @einar-saukas I think that Jhispter should allow people who wants possibly null Booleans to have them, and for the required booleans use the promitive type.
@jdubois as mentionned in my comment you just need to to generate a JD qith a required boolean to see the valudation problem which triggers à 500, do you want me to create such a repo? or do you want me to pull request the fix? or both?

[jdubois]

Having 3 possible values for a boolean definitely look wrong to be. If you have 3 values, you should use an enum.

[deepu105]

Yes I also agree with Julien. Having 3 value for Boolean sounds like a code
smell

On Sun, 11 Nov 2018, 7:43 am Julien Dubois <[email protected] wrote:

Having 3 possible values for a boolean definitely look wrong to be. If you
have 3 values, you should use an enum.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/jhipster/generator-jhipster/issues/8786#issuecomment-437647654,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDlF49K3VTvm7isjoGGQYzq7RNyIQ_6ks5ut8cggaJpZM4YXnfQ
.

[pascalgrimaud]

Agree with Deepu and Julien.

[DanielFran]

+1

[einar-saukas]

I understand your preference for required (not null) booleans. Even so, databases with nullable booleans (and the business requirements for them) do exist in practice. Why exclude them from JHipster?

Support for non-required boolean will make absolutely no difference for everyone that prefer to only declare required booleans, and increase JHipster usefulness for others. Also JDL behavior will be more predictable and regular, since the difference between required and non-required boolean will be exactly the same as other datatypes. Thus even if you won't use non-required boolean in your own applications, adding support to it can only have advantages, without any drawbacks.

[jdubois]

So you gave me some doubts here and I did some research:

• of course booleans should be true or false, that's the whole point of booleans, and having 3 states for a boolean doesn't make any sense. If you need three states, use an enum.
• of course when you unbox them you're going to have NPE. Having an NPE in your code is bad. And if you're lucky, instead of having a NPE your boolean will be resolved to false, so you'll lose your third state.

This is just bad in every way I can think of, and an abuse of autoboxing. In terms of UI, I've also seen that at a big bank, you needed to click 3 times on radio buttons: they could be true, false, or undefined. No user can ever make sense of this, and this was an endless cause of issues. Use a drop-down list instead.

[deepu105]

@yelhouti @einar-saukas we are sorry since the majority of core team agrees that this is not a good idea. We are already knee deep in open issues and we shouldn't focus on this. Our aim is not just to provide code but to provide code with best practices wherever possible.

[yelhouti]

@deepu105 I totally agree, i don't mind you guys always using primitive booleans even if understand einar's point. But closing the issue and keeping the wrapped booleans with 3 states is the opposite of best practice and is just the wrong choice IMO. I can switch everything to native booleans if you want

[deepu105]

@jdubois WDYT of switching to primitive booleans?

[jdubois]

Currently we are using Objects everywhere (so not the primitive types).

• There's probably a very negligible overhead of doing so (compared to a database call!)
• I know some people use "null" objects in order to reduce the output of their JSON, but taking the risks to have NPE everywhere for a performance reason looks like a weak argument to me (you'd rather you have view models or DTOs for that).
• If you read the Hibernate user guide they do recommend to use Objects for IDs (probably because they will then be used in the 1st and 2nd level caches), but they do use primitives for normal fields. I've also had a look at their (old) Caveat Emptor sample project, and it also uses primitives for normal fields (not IDs).

-> so as per our Policy 1 I would indeed advise to migrate to primitives. Oh, and that's logical with what has been said here, so that all looks good to me. Is everybody OK with that? If yes, this should be another ticket to do the migration.

[yelhouti]

@jdubois would you like me to work on this?

[jdubois]

@ yelhouti if you have the time, yes.
Could you create a specific issue for this?

[yelhouti]

@jdubois, I was answering you. I'll do and start working on it

[yelhouti]

@jdubois I'm writting the issue, and even thow I understand all your choices I think you don't realize the extend of the impact, specially on the database side:

• for NoSQL databases null might mean not present.
• for SQL if the not null is not enforced in the database, NPE may occur.
  I think we should encourage people to use only required booleans using a warning in the JDL for example, bit without forcing them.

Can we chat about this over some audio medium?

Thank for your understanding.

NB: I prefere moving to primitibe boooleans than keeping it this way.
PS: at the end of the call I will transcript all the arguments here.

[jdubois]

Hi @yelhouti sorry I can't easily do a phone call, but we can chat on the ticket, that's the way we usually work.

Could you open up the ticket, with your issues, and we'll chat there? I understand it might be more complex than expected, so let's it give it a try, but it doesn't have to succeed. We've been using Objects for 5 years and nobody had any issue with that until today, so that's probably not too bad.

[yelhouti]

@jdubois Sorry long day, I don't understand what you mean by open up the ticket with my issues (this is my ticket) do you want me to create another one? How would it be different from this one?

[deepu105]

It's fine. Just do the PR and refer this ticket

[jdubois]

Yes you can use this one, no problem, then I'm reopening it

[yelhouti]

Here is the PR, I decided at the end to give the user the right to choose if the boolean is required or not since, this way, I don't need to the databases scripts. Keeping columns that allow null values for primitive booleans could case NPE when reading from databases with boolean null values.
There many other reasons to giving the user the control of what he wants to generate, and having a required boolean is just a keyword away in the JDL, so users should be ok.

[yelhouti]

@jdubois after few tests it seems that using primitive types for Long is not a very good idea:
Since deserialization is done before validation check 0 is assigned to the int, and the @NotNull is triggered.
For booleans it's not a problem since false is set by default and a required boolean not defined or null is expected to be set automatically to false without triggering any validation error. for setting the int to 0 automatically and skipping validation it's another story.

[jdubois]

Thanks @yelhouti but I don't find it very coherent if we use booleans primitives but Integer and Long objects.

@vladmihalcea could you help us on this issue? From your opinion, what is the recommend way to use Hibernate: use primitive types (boolean, int, long) or objects (Boolean, Integer, Long)?

[yelhouti]

@jdubois I was doing my tests using version JHipster 4.X that used, spring 1.x which used an validator that implements api.validation 1.X, spring 6 uses hibernate-validator-6.X which checks the fields without using the java class java.beans.Introspect and therefore accessing the field directly without using getMyField or isMyBoolean which created the problem in the previous version. Therefore I can say there is no Need for my fix for jhipster 5.X, poeple still using 4.X like me can validate booleans adding a getter or even better moving to promitive types and removing the @NotNull annotation. I'm closing the issue and pull request.

[jdubois]

Thanks @yelhouti - I'm finding this quite surprising, but you know this better than me, clearly.
Then we still have this question of using primitives or Objects - I guess using primitives would still be better, but as this is also more complex than expected and probably causing a lot of breaking changes, let's not do it now. The current code has worked successfully for years, on lots of projects, and if nobody complained (until this ticket), then it's probably a good solution.

[yelhouti]

Use non primitive is better in my opinion, as using primitives set value to 0 if users sends null which creates odd and expected behaviors. Let's keep it this way, and keep up the good work guys :)

[vladmihalcea]

@jdubois Using wrappers allow you to cover NULL column types as well. Also for identifiers and versions is better to use wrappers as null means not assigned yet by Hibernate.

[jdubois]

OK thanks @yelhouti @vladmihalcea !!! Let's keep it this way, then.