jhipster / jhipster-core

JHipster Domain Language, used by JHipster UML and JDL-Studio to generate entities
Apache License 2.0
346 stars 116 forks source link

Problems with custom enum values #459

Closed 62mkv closed 4 years ago

62mkv commented 4 years ago
Overview of the issue

Currently, JDL has some support for specifying custom values for enum-s. Currently it only accepts Java identifiers as values (???)

So, not clear how to generate an enum that would look like:

public enum ProcessingStatus {
    IN_PROGRESS("In progress"),
    SUCCESS("Success"),
    FAIL("Fail"),
    UNDEFINED("Undefined");

    private String value;
...
}

also, faker template for such an enum will generate data that looks like this:

..., IN_PROGRESS (In_progress)
..., SUCCESS (Success)

Custom value is provided (sometimes) for human readable representation on the DB level, while keeping type-safe API for the code as well., so the values in the DB are expected to look like

..., Success
..., In progress
Motivation for or Use Case

See above

Reproduce the error

Reproduceable with this JDL:

enum ProcessingStatus {
    IN_PROGRESS (In_progress),
    SUCCESS (Success),
    FAIL (Fail),
    UNDEFINED (Undefined)
}
Related issues

https://github.com/jhipster/jhipster-core/issues/357

Suggest a Fix
JHipster Version(s)

6.8.0

JHipster configuration
Entity configuration(s) entityName.json files generated in the .jhipster directory
Browsers and Operating System
62mkv commented 4 years ago

.. and it also generates an enum that does not even compile:

package org.corp.enumeration;

/**
 * The ProcessingStatus enumeration.
 */
public enum ProcessingStatus {
    IN_PROGRESS (In_progress), SUCCESS (Success), FAIL (Fail), UNDEFINED (Undefined)
}
MathieuAA commented 4 years ago

Hello @62mkv, you can't do that in JHipster too using the Q&As.

Custom value is provided (sometimes) for human readable representation on the DB level

For such an added value, I think having underscores is enough and doesn't need to be a JDL-only feature (which would require work here and in the generator).

62mkv commented 4 years ago

what was this "(value)" added for than ? I mean, the generated enum does not even compile! what's the use for that ?

62mkv commented 4 years ago

when I raised the original issue, I had on mind only this case:

public enum ProcessingStatus {
    IN_PROGRESS("In progress"),
    SUCCESS("Success"),
    FAIL("Fail"),
    UNDEFINED("Undefined");

    private String value;

    ProcessingStatus(String value) {
        this.value = value;
    }

    public String getValue() {
        return value;
    }

of course, I had to be more specific, but I could not even imagine that it's going to be implemented... like it was !

62mkv commented 4 years ago

now, admittedly, there might be too many ways to (ab) use this functionality, so another idea: what if we could define "externalized" enums, like just saying:

enum ProcessingStatus 

entity Step {
 name String
 status ProcessingStatus
..
}

and have developer implement this enum on his own? this would at least save the hassle of JHipster redefining this enum every once in a while

62mkv commented 4 years ago

and the original issue is https://github.com/jhipster/jhipster-core/issues/357

MathieuAA commented 4 years ago

what was this "(value)" added for than ? I mean, the generated enum does not even compile! what's the use for that ?

Then there's a bug in the generator, you can submit an issue there (and/or submit a PR for that).

of course, I had to be more specific, but I could not even imagine that it's going to be implemented... like it was !

Okay, calm down. I understand you want an enhancement for custom enum values and you are annoyed because there seems to be a bug too in the generator (which I didn't know of, and didn't see in the generator issue tracker). The enhancement is about supporting values with spaces in them., and it wasn't the original scope. No big deal. Let's discuss it. My point was having underscores is enough.

...and have developer implement this enum on his own? this would at least save the hassle of JHipster redefining this enum every once in a while

You may want to look at the "side-by-side approach":

62mkv commented 4 years ago

I am aware of the side by side approach. it's an additional effort to what was considered to be rather trivial and generic development, which would make sense to have right in the generator.

the enhancement is not about "having values with spaces in them". the original idea was to have enums with a String property! "identifier" is not a String! so, this whole development. I don't even understand, how it could possibly be used?

again I sound as emotional )) but really, I feel like a complete idiot and it drives me mad ) no offense to anyone in thit great community of course!

MathieuAA commented 4 years ago

I've looked at the original issue and I remember it, and this wasn't mentioned anywhere. So yeah, I thought the example was sufficient because I'm no mind reader! Now, it's an enhancement to allow any strings too.

You talked about human-readability on the DB level, it's a matter of opinion and what goal one wants to achieve.

62mkv commented 4 years ago

Well, the original issue explicitly said

I would suggest to add a way to define custom text for each value like

and the example was even given with strings in quotation marks. And no questions were ever asked about that. You said yourself there that it's a "common use-case", so I assumed we're all on same page.

How "identifiers" came into this, I still can't imagine, maybe worth asking @colameo what was the supposed use-case for this implementation. Probably it is me who is missing mind-reading skill here... :(

62mkv commented 4 years ago

I still feel bad because of this so came back to already suspended laptop to write this:

  1. I absolutely appreciate any and all effort made by the community and especially by core maintainers such as @MathieuAA
  2. I fully understand that I am not entitled to anything here.
  3. What I've tried to highlight by raising this issue: the way it was implemented, I can't think of any possible way of meaningfully use those values - is it just me or is it the way it was implemented?
  4. So, in a constructive way, I would suggest the following:
    • improve the documentation so it's clear what those "values in enums" might be used for and what those will look like in generated code, so, whoever is going to use them, will know what they're up to
    • I will raise a separate issue to "add proper support for String-capable enums" (but I will need your assistance on this, as I am not sure against which or how many different projects this should be raised)

once again - thanks to the whole team, thank you @MathieuAA , and sorry if my comments occasionally sounded as not too polite or unnecessary emotional!

MathieuAA commented 4 years ago

Don't worry about it. There was a communication breakdown and misunderstanding from the very beginning -- in both issues, so it's better that you've created this issue instead of not doing it. So thank you.

I'm not against this feature, quite the opposite. I'm more interested in use-cases and needs than solutions.

My initial understanding of the need was to improve readability because enumerations without custom values are sometimes not clear. My motivation to implement this feature is clear from #357, and I've chosen the simplest solution to the problem: custom values (alphanumeric) + underscores. Perhaps it was only part of the problem and I didn't solve the other half. Let's try to do a better job here.

Say you have a pretty complex business logic with codes that mean nothing on their own without explanation (like acronyms or abbreviations). The custom enums could be used to provide a "full version" of the value. That was my original scope and idea, can you correct it?

62mkv commented 4 years ago

Like in

enum Battery {
AA(finger_sized)
AAA(toe_sized)
}

?

Might make sense in JDL context, but what's an added value in generated code ?

My original idea was specifically to allow for declaring String-enabled Enums, so that we could have this:

public enum RequestType {
    PRINT_PARCEL_REQUEST("PrintParcelRequest"),
    DELETE_PARCEL_REQUEST("DeleteParcelRequest"),
    CLOSE_SHIPMENTS_REQUEST("CloseShipmentsRequest");
    private final String value;

    RequestType(String value) {
        this.value = value;
    }

    public String getValue() {
        return value;
    }
}

out-of-the-box by just defining it as follows in the JDL:

enum RequestType **embedded String** {
 PRINT_PARCEL_REQUEST("PrintParcelRequest")
... etc 
}

where I suggest to make embedded a binary option for Enums, which could be given any JDL-supported type name as a value (String, Int, etc)

62mkv commented 4 years ago

I could even give it a go with PR if someone could give me initial guidance..

62mkv commented 4 years ago

another option could be to be able to define enum as "user-implemented", like as:

enum BatterySize external

entity Device {
 name string
 battery BatterySize
}

and the generated code for POJOs and such will just contain imported symbol for this Enum, which the end user will have to implement

62mkv commented 4 years ago

(again, I am only aware of tiny piece of the JHipster functionality so all such ideas should be validated by experts like you)

mshima commented 4 years ago

I took a look at generator part and custom enum values is completely broken. Current implementation is for frontend only. The java part should ignore the custom value, but it isn’t.

There are 2 approaches for custom values: 1 - Client side only. (Current implementation) 2 - Server side.

2 issues can be opened on generator-jhipster: Approach 1 as a bug. Approach 2 as a feature request.

Spaces on the value is another issue, I don’t know if it affects generator-jhipster side.

@62mkv can you open a bug report for approach 1 on generator-jhipster and add example with and without spaces on it?

62mkv commented 4 years ago

hi @mshima but @colameo says they're already addressing this in https://github.com/jhipster/jhipster-ide/issues/340 ? I don't want to create more noise than otherwise necessary :)

MathieuAA commented 4 years ago

I'm working on the issue in the generator

mshima commented 4 years ago

@62mkv if you really want custom enum value on the server side, you should open the feature request on generator-jhipster.

62mkv commented 4 years ago

first: https://github.com/jhipster/generator-jhipster/issues/11585

62mkv commented 4 years ago

second: https://github.com/jhipster/generator-jhipster/issues/11586

MathieuAA commented 4 years ago

Does the second one concern the bug or the "any string"?

62mkv commented 4 years ago

it is a feature request. concerns "any string"

it was replying to @mshima :

if you really want custom enum value on the server side, you should open the feature request on generator-jhipster.

MathieuAA commented 4 years ago

I'm gonna create the issue about enum values, the fix is done.

MathieuAA commented 4 years ago

Work on allowing a broader range of custom string values is done in #460 (I'm reviewing my work, I think I may add more tests for some cases).

MathieuAA commented 4 years ago

The solution:

Now, another issue is that the generator using the fieldValues attribute in entity JSON files to list enum values, in a string form instead of an array. Which, in turn, forces JDL not to accept the , char inside custom enum values (not a big deal). I'm not in favor of working to allow this character as the added value is almost nil (I've other reservations but this is my biggest one).

MathieuAA commented 4 years ago

@colameo I've sent you a message on Gitter the other day about that. What do you think about that? Is it okay for you? I'm waiting for you before making it "official" (documenting, etc.)

colameo commented 4 years ago

@MathieuAA of course I need to adapt this to make it work but as you can see below it would be possible and therefore I've created an issue to fix this:

Screenshot 2020-04-26 at 11 43 44

colameo commented 4 years ago

btw, what about this:

Screenshot 2020-04-26 at 11 47 57

and do we allow to mix different custom values like in MyEnum1 too (I mean should we have a linting validation with a warning or so for mixed values)?

colameo commented 4 years ago

...I think it makes sense to fix this too and allow numeric values as well:

Screenshot 2020-04-26 at 11 59 12

MathieuAA commented 4 years ago

btw, what about this:

Screenshot 2020-04-26 at 11 47 57

and do we allow to mix different custom values like in MyEnum1 too (I mean should we have a linting validation with a warning or so for mixed values)?

Let's not support everything Java allows... Strings and integers should be supported, at least.

62mkv commented 4 years ago

may I ask, why (or how) are "java identifiers" useful in this context? why not just allow only Java literals (Int/String) as custom Enum element values?

62mkv commented 4 years ago

if identifier purpose is to represent an externally defined constant, then it might make sense to have it defined as an FQDN java name com.example.Constants.CONSTANT_NAME ?

MathieuAA commented 4 years ago

may I ask, why (or how) are "java identifiers" useful in this context? why not just allow only Java literals (Int/String) as custom Enum element values?

@62mkv good question, here's an answer. If the point of having specific enum values is to be able to read them as any text, then it may be a mistake: it's an i18n issue, not a back-end issue. After all, why not allow almost any string?

However, java identifiers are a compromise for enums, remember weird business code = helpful description. Allowing any string is the short-term goal (there's a PR for that) but it comes with a trade-off: a one-time breaking change in the JDL, but it would allow us to distinguish between custom values as strings and custom values as integers.

Supporting integers are the next step.

As for constants, it's a different implementation as one could put them in interfaces or plain final classes, it's a debate that's beneath the JDL's concern. The JDL isn't Java, nor Javascript. This question could be asked in the generator, but I don't see the point frankly.

62mkv commented 4 years ago

not sure I understood, but probably what I am really after is addressed in https://github.com/jhipster/generator-jhipster/issues/11586 so no point in discussing that here too much. I would just hope that what is being done in this issue will eventually be usable for #jhipster/generator-jhipster/11586 as well

MathieuAA commented 4 years ago

the payload keyword bothers me because I don't get it yet, that's the only issue I have

MathieuAA commented 4 years ago

nevermind, I got it. must be too tired

MathieuAA commented 4 years ago

The PR has been merged. To sum up the next steps: