Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Database validation in jdl #12975

Merged
merged 5 commits into from
Nov 11, 2020
Merged

Remove Database validation in jdl #12975

merged 5 commits into from
Nov 11, 2020

Conversation

nicolas63
Copy link
Member


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (bellow reviewers) and adding skip-ci label, you can still see CI build result at your branch.

fix #12959

@MathieuAA
Copy link
Member

Validation as a whole should be disabled when using blueprints.... Not just that.

@nicolas63
Copy link
Member Author

sorry for my misunderstanding, how can I know if a blueprint is used in this part?

@MathieuAA
Copy link
Member

@nicolas63 don't be! I'm gonna walk you through how it can be done step by step.

If you look at the jdl/jdl-importer.js file, you'll see it's the place where the actual JDL importing is done.
From there, there are two paths:

  • the first one for importing JDL without applications,
  • the second one reserved to the opposite: importing JDL with applications (at least one)

Now, go line 179. you'll see this function:

function checkForErrors(jdlObject, configuration) {
    let validator;
    if (jdlObject.getApplicationQuantity() === 0) {
        let application = configuration.application;
        if (!application && doesFileExist('.yo-rc.json')) {
            application = readJSONFile('.yo-rc.json');
        }
        let applicationType = configuration.applicationType;
        let databaseType = configuration.databaseType;
        let skippedUserManagement = configuration.skipUserManagement;
        if (application && application['generator-jhipster']) {
            if (applicationType === undefined) {
                applicationType = application['generator-jhipster'].applicationType;
            }
            if (databaseType === undefined) {
                databaseType = application['generator-jhipster'].databaseType;
            }
            if (skippedUserManagement === undefined) {
                skippedUserManagement = application['generator-jhipster'].skipUserManagement;
            }
        }
        validator = JDLWithoutApplicationValidator.createValidator(jdlObject, {
            applicationType,
            databaseType,
            skippedUserManagement,
        });
    } else {
        validator = JDLWithApplicationValidator.createValidator(jdlObject);
    }
    validator.checkForErrors();
}

This function checks a passed JDL object for errors. This is the validation is done. When there's no blueprint, the actual rules for validations are implemented and it works.
When there are blueprints, one can augment the JHipster scope to add databases, field types, etc. but they will trigger an error from this function, as you noticed.

There are two fixes to do:

  • a first one for JDLs without apps,
  • another one for JDL with apps.

There are two files: jdl/validators/jdl-without-application-validator.js and jdl/validators/jdl-with-validator.js that we need fix.

For the "no app" case, we need to pass to the validator whether there are blueprints involved, which is done line 173 of the jdl-importer.js file where we need to add one more parameter to the applicationSettings: blueprints: ... like this:

function checkForErrors(jdlObject, configuration) {
    let validator;
    if (jdlObject.getApplicationQuantity() === 0) {
        let application = configuration.application;
        if (!application && doesFileExist('.yo-rc.json')) {
            application = readJSONFile('.yo-rc.json');
        }
        let applicationType = configuration.applicationType;
        let databaseType = configuration.databaseType;
        let skippedUserManagement = configuration.skipUserManagement;
        let blueprints = configuration.blueprints; // first there
        if (application && application['generator-jhipster']) {
            if (applicationType === undefined) {
                applicationType = application['generator-jhipster'].applicationType;
            }
            if (databaseType === undefined) {
                databaseType = application['generator-jhipster'].databaseType;
            }
            if (skippedUserManagement === undefined) {
                skippedUserManagement = application['generator-jhipster'].skipUserManagement;
            }
            if (blueprints === undefined) { // here
                blueprints = application['generator-jhipster'].blueprints;
            }
        }
        validator = JDLWithoutApplicationValidator.createValidator(jdlObject, {
            applicationType,
            databaseType,
            skippedUserManagement,
            blueprints, // and there
        });
    } else {
        validator = JDLWithApplicationValidator.createValidator(jdlObject);
    }
    validator.checkForErrors();
}

and finally in the jdl/validators/jdl-without-application-validator.js file we need to:

  • add the new blueprints param to the documentation,
  • and add an if to the createValidator function like this:
function createValidator(jdlObject, applicationSettings = {}, logger = console) {
    if (!jdlObject) {
        throw new Error('A JDL object must be passed to check for business errors.');
    }

    if (applicationSettings.blueprints && applicationSettings.blueprints.length !== 0) {
      return { checkForErrors: () => {} };
    } 

    return {
        checkForErrors: () => {
            checkForEntityErrors();
            checkForRelationshipErrors();
            checkForEnumErrors();
            checkDeploymentsErrors();
            checkForOptionErrors();
        },
    };

  // ...
}

That's the hardest part. The easiest part will be updating the jdl-with-application-validation.js file:

function createValidator(jdlObject, logger = console) {
    if (!jdlObject) {
        throw new Error('A JDL object must be passed to check for business errors.');
    }

    return {
        checkForErrors: () => {
            checkForApplicationErrors();
            jdlObject.forEachApplication(jdlApplication => {
                const blueprints = jdlApplication.getConfigurationOptionValue('blueprints');
                if (!blueprints || blueprints.length > 0) {
                    return;
                }
                checkForEntityErrors(jdlApplication);
                checkForRelationshipErrors(jdlApplication);
                checkForEnumErrors();
                checkDeploymentsErrors();
                checkForOptionErrors(jdlApplication);
            });
            checkForRelationshipsBetweenApplications();
        },
    };

    // ...
}

There will be some testing to do: check no validation is performed when blueprints are passed in the two cases. This should be done at the highest level: for the jdl-importer file and for the two validators.


Hope it wasn't too long :) if you want to do this PR, it'd be awesome!

@nicolas63
Copy link
Member Author

thank you @MathieuAA for your precious explanation, I will make the changes.

@mshima
Copy link
Member

mshima commented Nov 8, 2020

Print a warning:

      return { checkForErrors: () => {
          console.log('Generating application with blueprint, skipping jdl validation');
      } };

@MathieuAA
Copy link
Member

Maybe not console.log, but console.warn :)

@nicolas63
Copy link
Member Author

@MathieuAA
There will be some testing to do: check no validation is performed when blueprints are passed in the two cases. This should be done at the highest level: for the jdl-importer file and for the two validators.

how can i do this ? i have try to spy logger but the method createValidator don't use
const logger = require('../utils/objects/logger');

should I use it rather than console in function createValidator(jdlObject, logger = console) ?

@MathieuAA
Copy link
Member

In the tests, pass whatever log object you want to test like it's done l.85 of the test/jdl/validators/jdl-with-application-validator.spec.js file. You can use spies too.

@nicolas63
Copy link
Member Author

@MathieuAA i have added UT but some checks failed during CI, it's due to my modification ?

@MathieuAA
Copy link
Member

I'll check right away

@MathieuAA
Copy link
Member

It looks like an issue with Actions as a whole, I'm looking into it

Copy link
Member

@MathieuAA MathieuAA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice :) only a few changes needed and it's good to go.

jdl/validators/jdl-without-application-validator.js Outdated Show resolved Hide resolved
jdl/validators/jdl-with-application-validator.js Outdated Show resolved Hide resolved
jdl/jdl-importer.js Show resolved Hide resolved
test/jdl/jdl-importer.spec.js Outdated Show resolved Hide resolved
test/jdl/validators/jdl-with-application-validator.spec.js Outdated Show resolved Hide resolved
@MathieuAA
Copy link
Member

@MathieuAA i have added UT but some checks failed during CI, it's due to my modification ?

There was an issue with GH actions (I've noticed warning about 429 errors) and restarting the tests worked like a charm.

@MathieuAA
Copy link
Member

@nicolas63 thanks!! Feel free to merge it once the CI report goes green :)

@nicolas63
Copy link
Member Author

thanks to you @MathieuAA for your advice ! Can you merge it ? I don't have the rights to do

@MathieuAA MathieuAA merged commit 819d07f into jhipster:main Nov 11, 2020
@pascalgrimaud pascalgrimaud added this to the 7.0.0-beta.0 milestone Dec 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Customize database validation in JDL
4 participants