Add "unique" field validation

[taurus227]

This is a pull request for single-field unique constraints, as discussed in this issue: Adding commands for unique constraints

It includes fixes for the findings detailed in the previous pull request.

[gmarziou]

Thanks.
As a bonus, maybe it would be useful to configure one entity with such constraint in definitions used by Travis builds.

[ctamisier]

Should we add the front-end part as well ?, Like we have in the user creation/modification dialog with the login.

[deepu105]

How would you do front end without doing a query to check each input which is an overhead?

…

On 11 Dec 2017 9:11 pm, "Clément Tamisier" ***@***.***> wrote: Should we add the front-end part as well ?, Like we have in the user creation/modification dialog with the login. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6850 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABDlF-dw_WB31Ni7Buz7TgBJFygTTr5Rks5s_YxmgaJpZM4Q9zgE> .

[ctamisier]

For me we already have this use case with the login and email field of a user.
We perform the check of an eventual already existing login or email in the database and returning a zalando problem.

I'm just thinking it has to be same if we want to handle unicity in entities.

[taurus227]

@gmarziou Yes, I tested it by adding unique to one of the entities and manually checking the generated liquibase file, but I wasn't sure if I should commit that change. I will add it. However, I was actually looking for a proper automated test, where the generated liquibase file would actually be compared against an expected liquibase file, instead of visually checking it. Do you have such a test somewhere?

@ctamisier Adding the unique constraint will enforce uniqueness on the DB level, but I have no clue how the error would be shown to the user. I was hoping it would just "automagically" work :) Maybe the exception would be handled on the server side by the global exception handler, translated to a zalando problem, which would then be properly handled on the client side? If changes are needed for this to work, I can do it. However, I'm very new to JHipster. Could you please point me to the actual code that needs to be changed?

[taurus227]

@gmarziou I pushed a change to Division.json. Ran travis/./build-samples.sh generate app-ng2-psql-es-noi18n and manually checked the generated liquibase file.

However, I'd like to add an automated test for this. I'm thinking to:

• Create a

  travis/samples/app-ng2-psql-es-noi18n-test

  directory
• Make sure the build-samples.sh script skips such directories
• Save the expected liquibase file 20170626061520_added_entity_Division.xml in the -test directory
• Add a test case to compare the expected file with the generated
  travis/samples/app-ng2-psql-es-noi18n-sample/src/main/resources/config/liquibase/changelog/20170626061520_added_entity_Division.xml
  file

Any idea where exactly could I add such a test case?

Or maybe my approach is completely wrong, we should check that the constraint works by adding some kind of higher-level test, where we attempt to create another Division with the same name?

[deepu105]

@taurus227 @ctamisier no we don't want to handle this in the UI as it's complicated to generate and that's why I was originally against adding this validation type. IMO this is already good enough if you want to enforce the uniqueness of a field but if you want more like client-side validation and nice messages I guess you have to implement this based on your business need.

[deepu105]

@taurus227 its not nice to mix Travis tests and unit tests, if you want to unit test this I suggest to take a look at the karma tests, you can do comparisons there easily, look at the test-entity.spec
But if you want to test if unique attribute in the changelog works I guess its not needed as its a liquibase feature that you are testing and not our code

[deepu105]

I'm merging it as its good enough for what we originally agreed to support.

@taurus227 can you update the docs in jhipster.tech and may be update jhipster-core to support this in JDL?
Ping @MathieuAA

[MathieuAA]

@deepu105 roger that

[taurus227]

@deepu105 Created a pull request for the documentation

[taurus227]

@deepu105 My goal is to unit test my own changes in _added_entity.xml, the code that generates the liquibase changelog. I want to test that if Division.json contains unique for a field, the liquibase changelog will also contain a unique constraint for that field. If I understand it correctly, Karma is at the UI level, which is way higher level than a unit test. I haven't seen any unit tests for the code that generates the changelog file.

[deepu105]

Sorry yes Karma is not exactly unit tests, but you can indeed do this in test/entity.spec.js as you can generate an entity and verify the generated in memory files see this for example

describe('AngularJS', () => {
            beforeEach((done) => {
                helpers.run(path.join(__dirname, '../generators/app'))
                    .withOptions({ skipInstall: true })
                    .withPrompts({
                        baseName: 'jhipster',
                        packageName: 'com.mycompany.myapp',
                        packageFolder: 'com/mycompany/myapp',
                        serviceDiscoveryType: false,
                        authenticationType: 'jwt',
                        hibernateCache: 'ehcache',
                        databaseType: 'sql',
                        devDatabaseType: 'h2Memory',
                        prodDatabaseType: 'mysql',
                        useSass: false,
                        enableTranslation: true,
                        nativeLanguage: 'en',
                        languages: ['fr'],
                        buildTool: 'maven',
                        enableSocialSignIn: false,
                        clientFramework: 'angular1',
                        skipClient: false,
                        skipUserManagement: false,
                        serverSideOptions: []
                    })
                    .on('end', done);
            });

            it('creates expected default files', () => {
                assert.file(expectedFiles.server);
                assert.file(expectedFiles.jwtClient);
                assert.file(expectedFiles.jwtServer);
                assert.file(expectedFiles.maven);
                assert.file(expectedFiles.dockerServices);
                assert.file(expectedFiles.mysql);
                assert.file(getFilesForOptions(angularJsFiles, {
                    useSass: false,
                    enableTranslation: true,
                    serviceDiscoveryType: false,
                    authenticationType: 'jwt',
                    testFrameworks: []
                }));
                assert.noFile([
                    `${TEST_DIR}gatling/conf/gatling.conf`,
                    `${TEST_DIR}gatling/conf/logback.xml`
                ]);
            });
            it('contains clientFramework with angular1 value', () => {
                assert.fileContent('.yo-rc.json', /"clientFramework": "angular1"/);
            });
            it('contains clientPackageManager with npm value', () => {
                assert.fileContent('.yo-rc.json', /"clientPackageManager": "yarn"/);
            });
            it('contains install-node-and-yarn in pom.xml', () => {
                assert.fileContent('pom.xml', /install-node-and-yarn/);
            });
            shouldBeV3DockerfileCompatible('mysql');
        });

[cbornet]

This has problems with our tests : if an entity A has 2 required relationships to the same entity B and B has a unique field. We already had the case with User that has login as unique field and we had to add random data to the login

[taurus227]

@cbornet Sorry, I don't understand what you mean about the relationships between A and B. Could you give a more detailed example?
About generating random data: it seems to me that the tests are not written taking into account that a certain field has to be unique. That would be a problem with the test, not a problem with the feature. If you would like help fixing the tests, I can do that, just point me to what's failing.

[jdubois]

I'm sorry I didn't follow this, and I have some issues with this code:

• The "validation" part is supposed to work with Angular/React on the client side, on Bean Validation on the Java side, and with database constraints. Here, we only take care of database constraints: that means that when the user uses a value that is already used, he'll get an error message from database constraint that was violated. For me, we should have:
  • a first check to see if that's possible (not only rely on database constraints)
  • custom error messages for this (and I don't see any custom messages in the PR)
  • specific Karma and Protractor tests for this
• This code doesn't check for non-SQL databases, where constraints do not apply. I just tested (to be sure) with MongoDB: I could create a "unique" field, but then when I ran my application I could add twice the same field. It just doesn't work at all here.
• I'm also not totally sure for @cbornet 's comment, so I'll let him detail, but I'm pretty sure he is correct

-> for me this is too early to release this. My proposal is simply to remove this in the question, so that people don't "see" it, but keep the rest of the code. As I'd like to do a release today, please forgive me if I force this, we can still put the question back easily.

[taurus227]

@jdubois In the original issue @deepu105 mentioned that "To really validate something is unique before saving it you need to compare it against existing items to determine if unique which is costly and tricky operation." I agree. And even if you do that, you will still have a concurrency risk: somebody might add the same value after you have checked if it exists, but before you try to commit your transaction, so you still need the constraint and you still might only catch the duplicate value at commit time with that constraint. So the constraint violation error should be handled nicely (with a user-friendly error message) even if a check would be implemented at the UI level. I would argue that most use cases would generate duplicate values rarely, so the check at the UI level adds value rarely in exchange for a performance overhead that will always be there, so I would only implement the constraint at the DB level and if somebody has a use case that creates duplicates often, they can add the extra query to that workflow.

I know nothing about MongoDB, but it seems it has unique indexes as well: https://docs.mongodb.com/v3.2/tutorial/unique-constraints-on-arbitrary-fields/
I'll look into implementing it if needed. How about Cassandra and Couchbase, I guess those are also needed?

[jdubois]

@taurus227 thanks, let me take my points in order:

• Fair point, let's keep it like that. Still, I'd like to have some specific "hint" in the UI, and some custom error message.
• Don't worry for other databases for the moment: for MongoDB that can be added later, and for Cassandra & Couchbase just don't do it (at least Cassandra doesn't have this concept, but anyway both of them have <1% users, so it's not worth the effort)

[taurus227]

What does "hint" mean? Some "sign" that would let the user know that we expect him to enter a unique value, similar to a star showing that a field is required? Or just some instant feedback, like when entering a user name on some websites, it displays "Available" or "Already exists"?

[jdubois]

Yes, I meant some sort of visual indicator, but I understand this is too vague and this is going to take time to decide - so let's forget about it. What happens currently when there is an error, do you have some kind of server-side error that pops up?

[cbornet]

I don't understand what you mean about the relationships between A and B. Could you give a more detailed example?

I created #6860

[deepu105]

This is why I was originally against adding a unique validation, and somewhere along the line somebody convinced me :P I forgot who and how, but given the issues, I would vote to revert this for now and if there is a nice solution addressing all concerns we can consider it back Thanks & Regards, Deepu

…

On Wed, Dec 13, 2017 at 2:23 PM, Christophe Bornet ***@***.*** > wrote: I don't understand what you mean about the relationships between A and B. Could you give a more detailed example? I created #6860 <#6860> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6850 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABDlF2IA_jtC44uEcPyY9AkTPGB5g2sLks5s_8-1gaJpZM4Q9zgE> .

[taurus227]

Don't kill it now... That would mean this will never be done.

[agoncal]

I have plenty of emails, slugs, etc. in my database, and wouldn't mind a "unique" feature like this one. A database error message is good for me for now, it could be improved in feature released.

My 2 cents

[jdubois]

I'm going to do a release "anytime soon", and as this isn't 100% complete I'm going to comment out the question in the sub-generator. I will link my commit here, and it will then be easy to roll-back it.

[ctamisier]

Internal server error. was displayed is the UI.

having this http response:

{
  "type" : "http://www.jhipster.tech/problem/problem-with-message",
  "title" : "Internal Server Error",
  "status" : 500,
  "detail" : "could not execute statement; SQL [n/a]; constraint [job_code_key]; nested exception is org.hibernate.exception.ConstraintViolationException: could not execute statement",
  "path" : "/api/jobs",
  "message" : "error.http.500"
}

[jdubois]

Thanks @ctamisier -> so if we just rely on database constraints (which is not optimal and only works with SQL databases, but as we said it's much easier), then we should at least have a nice error message for this.

[taurus227]

@jdubois I mentioned earlier that MongoDB also has unique constraints. Yes, totally agree, I think we should return a zalando problem to have a nice error message displayed on the UI side?

[jdubois]

@taurus227 well if you can do MongoDB that great, but on a first release there's no need to support everything
For the errors: yes for the "zalando problem", and on the UI side you need to have a look at our "jhi-alert" directive. Same thing as the database: you can only do Angular for the moment (AngularJS is dying, React is still not finished)