mff-uk / odcs

ODCleanStore
1 stars 11 forks source link

Validator for RDF Loader + other isValid method implementations #330

Open tomas-knap opened 11 years ago

tomas-knap commented 11 years ago

Jirka,, when examining the code:

   public String SPARQL_endpoint = "";

    public String Host_name = "";

    public String Password = "";

    public List GraphsUri = new LinkedList<>();

    public WriteGraphType Options = WriteGraphType.OVERRIDE;

    @Override
    public boolean isValid() {
        return SPARQL_endpoint != null && 
                Host_name != null && 
                Password != null && 
                GraphsUri != null && 
                Options != null;
    }  

The isValid method returns always true, because all the variables are not null. This is not desired, right?

Please correct also for the other DPUs.

tomesj commented 11 years ago

I dont known about this method isValid() - data validation a their method isValid() I have implemented in dialogs (in *Dialog.class). yet.

I dont understand the importance and using this method - I was created by Peter. Why again ?

Parameters as SPARQL_endpoint, Host_name,...are set and validated in dialog yet.

May be there is another importance. Should I set it parameters to null as default or how to change it ? I have no ideas.

tomas-knap commented 11 years ago

If you set params to null, then it would cause the problem when configuration is loaded to the dialogs I guess. But you can improve the isValid() method, so that it also checks that !Sparq_enpoint.isEmpty() for example

tomas-knap commented 11 years ago

You did not implement the isvalid method, jirka?

tomesj commented 11 years ago

No I didnt implement this method - I use validation only in dialogs based on my classes for validation and validation of vaadin components.

tomesj commented 11 years ago

Only something like:

@Override public RDFLoaderConfig getConfiguration() throws ConfigException { if (!comboBoxSparql.isValid() | !areGraphsNameValid()) { throw new ConfigException(ex.getMessage(), ex); } else { saveEditedTexts(); RDFLoaderConfig config = new RDFLoaderConfig(); String graphDescription = (String) optionGroupDetail.getValue(); WriteGraphType graphType = getGraphType(graphDescription); config.Options = graphType; config.SPARQL_endpoint = (String) comboBoxSparql.getValue(); config.Host_name = textFieldNameAdm.getValue().trim(); config.Password = passwordFieldPass.getValue(); config.GraphsUri = griddata;

        return config;
    }
}
kukharm commented 11 years ago

I didn't implement this method too, i used validation similar way as Jirka. Petyr is author of RDFExtractorConfig.java (from which part of the code in the first comment). So i redirect to him.

skodapetr commented 11 years ago

Yes I'm the one who is responsible ... but this code is only very basic implementation which can go wrong for example if DB record is corrupted .. this should only prevent DPU failed on null pointer exception noting more.

Similar code is in every configuration. If DPU require more functionality then this need to be updated. Unfortunately I do not know specification and requirements on DPU's configuration.

Also the question is if advanced validation can be done here, I guess we have try out.

So I reassign back to Jirka. Jirka please provide specification for advanced validation .. and then we can update this to be more useful, or you can update this method your self. It should be hard and as the author of the RDF you now best the limitations.

skodapetr commented 11 years ago

Tomas (Jirka): we should discuss the placement and role of the validation for isValid method and configuration dialog.

tomesj commented 11 years ago

I think that here is the validation unnecessary. Required validation is performed directly on the form and does not store thus invalid data.

skodapetr commented 11 years ago

Yes but imagine following situation: you add new field to the configuration .. replace the DPU .. and then the invalid configuration is set and DPU throws null pointer exception. IMHO is better to use such code to prevent this.

Also there is requirements that after replace every configuration should be re-validate. If all the validation code is in dialog .. the this would be meaning less.

But the true is that from dialog we can better interact with the user, like show nice message .. give suggestion and so on ..

tomesj commented 10 years ago

I think, that this issue is not actuall yet and could be closed :-)

tomas-knap commented 10 years ago

Just to recap, isValid method is calling when loading data to DPU configuration dialog and also when running DPU?

tomesj commented 10 years ago

This method is Valid in configuration of DPU is called only when loading data to DPU dialog. Each dialog has then it´s own parameter validation placed directly in dialog - it stop saving invalid configuration.

Thanks that, when DPU is running, configuration is valid yet. But parameters are checked in called DPU method (for sure) once again - but as part of called method - this isValid method is not been used yet.

skodapetr commented 10 years ago

@tomesj as you are the author of last comment, can I ask you what I'm suppose to do with this issue?

tomesj commented 10 years ago

Maybe Tomas assigned you, I don´t know anything about that. Try to ask him.

tomas-knap commented 10 years ago

I do not know. But Petr, could you please comment the isValid method a bit in the code, so that it is clear why null value is checked? Also if not explained, explain in the code when isValid is called.

Then we can close I think