Skip to content

Commit

Permalink
A small (2 line) fix for 3 or 12 (depending on how it is counted) tes…
Browse files Browse the repository at this point in the history 
…ts + removal at least one error.

The problem was that when a 'parents' attribute is added, it was verified possibly wrongly.  The constraints said that this should be a list of classes.  The list constraint passed, but the classes constraint was not covered and failed.
  • Loading branch information
@stdotjohn
stdotjohn committed Nov 4, 2022
1 parent c738990 commit 0e09816
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions lib/goo/validators/enforce.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ def self.enforce(inst,attr,value)
errors_by_opt = {}
enforce_opts.each do |opt|
case opt
when :class
nil
when :unique
unless value.nil?
dup = Goo::SPARQL::Queries.duplicate_attribute_value?(inst,attr)
Expand Down

5 comments on commit 0e09816

@syphax-bouazzouni
Copy link

Choose a reason for hiding this comment

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

Hi @stdotjohn ,

I agree with your problem discription "the classes constraint was not covered", but i don't think that disabling it is the fix (and may break the ontologies_linked_data models).

For information also, here the constrain :class for an attribute means it will be instanciate as a Class resource (owl:Class , not the ruby class)

This behavior is done the else state of the switch

goo/lib/goo/validators/enforce.rb

Lines 98 to 113 in 0e09816

else
model_range = opt.respond_to?(:shape_attribute) ? opt : Goo.model_by_name(opt)
if model_range and !value.nil?
values = value.kind_of?(Array) ? value : [value]
values.each do |v|
if (!v.kind_of?(model_range)) && !(v.respond_to?(:klass) && v[:klass] == model_range)
add_error(model_range.model_name, errors_by_opt,
"`#{attr}` contains values that are not instance of `#{model_range.model_name}`")
else
if !v.respond_to?(:klass) && !v.persistent?
add_error(model_range.model_name, errors_by_opt,
"`#{attr}` contains non persistent models. It will not save.")
end
end
end
end

@mdorf can give a second review of this

@ncbo-deployer
Copy link

@ncbo-deployer ncbo-deployer commented on 0e09816 Nov 18, 2022 via email

Choose a reason for hiding this comment

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

@syphax-bouazzouni
Copy link

Choose a reason for hiding this comment

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

I didn't have time to test it localy, but I think the error would be in this call http://data.bioontology.org/ontologies/CL/classes?display=parents, where the parents would be no more instantiated as Classes but only URIs.
But maybe I'm wrong (and I'm sorry if its the case)

@stdotjohn
Copy link
Author

Choose a reason for hiding this comment

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

After looking over the else case more closely, I agree with your diagnosis. I will write a test in ontologies_linked_data (or in goo) to distinguish the solutions. I have also (finally) found a fixx though I am not sure that it is best.

The problem is that there is more that one model named :class and Goo.model_by_name is returning the wrong one in the failing tests. There is no provision in the code for having more than one model with a given name. There are several solutions for this:

  • making the code understand how to handle multiple models with the same name (unnecessary from other perspectives than the tests),
  • making there only be one model with a name (requires a merge of at least two models), or
  • add a slightly erroneous call to Goo.add_model (would have been a private method and un-callable in java) and adding an explanation to each instance,
    to name a few. I decided on the last questionable approach because it was easy to do. It will be committed and pushed soon.

@syphax-bouazzouni
Copy link

Choose a reason for hiding this comment

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

Hi,
Sorry for my late reponse, I agree totaly on your diagnosis (and observed it locally). Model name :class was used for two models in the file test_model_complex.rb as Term and test_schemaless.rb as Klass.

So its more a bug in the tests than the code.

I suggest to (I'm not sure if it is exactly what you proposeed to do, if it's the case great 👍):

  • Fix the test first by changing the model name of Term to :term
  • in the code (Goo.add_model) raise an Exeception if a Model had already the same name
  • add a test (in goo) for to test that this new Exception is raised

Please sign in to comment.