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

Latest validate missing label extension warning #546

Closed
Tracked by #30
jpl-jengelke opened this issue Nov 9, 2022 · 10 comments · Fixed by #559
Closed
Tracked by #30

Latest validate missing label extension warning #546

jpl-jengelke opened this issue Nov 9, 2022 · 10 comments · Fixed by #559
Assignees
Labels
B12.1 B13.1 bug Something isn't working i&t.done i&t.issue invalid This doesn't seem right

Comments

@jpl-jengelke
Copy link
Contributor

🐛 Describe the bug identified during I&T

Earlier tags of validate, e.g. 2.3.0, included a warning message when the --lblx-extension flag was used and the file type (e.g. xml or lblx) was not found.

WARNING  [warning.file.not_referenced_in_label]   File is not referenced by any label

At one point during earlier release cycles, only the warnings appeared, but ticket discussions indicated that an ERROR should be returned when the extension was not matched.

The current release versions (2.4.0 and 3.0.2) only display the following message:

FAIL: gov.nasa.pds.validate.ValidateLauncher
      ERROR  [error.execution.no_products_found]   No Products found during Validate execution. Verify arguments, paths, and expected label extension. See documentation for details.

The missing label warnings disappeared. This ticket is to either acknowledge the warnings should not appear or to return them to the Validate report.

🥼 Related Test Case(s)

B13.0, VAL.4: #524

🔁 : Related issues

#521
#496
#482

➕ Additional Details

📜 To Reproduce

Steps to reproduce the behavior:
Using Validate 3.0.2 compared to Validate 2.3.0 (use dataset uploaded to 524):

validate -R pds4.bundle testdata/bundle2/bundle_kaguya_derived.xml --label-extension lblx

🕵️ Expected behavior

The WARNING appears in addition to the ERROR.

📚 Version of Software Used

Validate 3.0.2
Validate 2.3.0

🩺 Test Data / Additional context

https://github.com/NASA-PDS/validate/files/9969122/testdata.zip (See validate#524)

🏞Screenshots

🖥 System Info

  • OS: Mac OS X 11.7.1
  • java 11.0.3 2019-04-16 LTS
  • Validate 2.4.0 or 3.0.0
  • GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin20)

🦄 Related requirements

⚙️ Engineering Details

@nutjob4life
Copy link
Member

Whew, okay, finally figured out the cause. Goodness, validate is much more complex than I ever surmised!

The change in behavior was put in place by 894024d where @jordanpadams removed the RegisterTargets command from the BundleValidationRule (and also the CollectionValidationRule).

With this command, validate would look for the referenced files but not find them since they're .xml files and we used the --label-extension lblx option to say "Hey, look for .lblx files instead". In validate-2.3.0 that produces the nice warning "File is not referenced by any label".

But in validate-3.0.2 and later, the warnings are gone. And the reason is: we never took the time to register targets to look over, so we're looking at an empty set of targets—which of course validate just fine.

The fix might be as simple as putting RegisterTargets back into BundleValidationRule (and CollectionValidationRule). But I want to let @jordanpadams explain why it was removed in the first place.

@viviant100
Copy link

Kudos for finding the root cause, @nutjob4life!

@nutjob4life
Copy link
Member

I have my moments @viviant100 ☺️

@jordanpadams
Copy link
Member

@nutjob4life great catch.

as a quick intro, a couple of the goals of my refactoring besides the original bug fix was to (1) remove duplicate crawling of files / directories and (2) remove duplicate reads of the same file. but I may have gotten a little too aggressive with that in some cases as we are unraveling.

Here the reason I changed this aspect of the code. Using BundleValidationRule as the example, here is what the previous functionality did:

  1. per your note, the Rule class would call RegisterTargets
  2. then kickoff the pds4.bundle command chain
  3. which would kickoff LabelInFolderRule
  4. which would kickoff pds4.label command chain
  5. which includes a step to call RegisterTargets again

at the time I was refactoring, I thought that the code may have been crawling the same files/directories multiple times, and was no longer necessary. but per this bug, it may not be the case. I can't quite remember how deep RegisterTargets crawls or how that functionality works exactly.

in general, I think the code is super complicated in how it crawls, registers, and tracks files that are entering the command chains. it seems like in some cases, the code tries to validate and register files and directories as soon as they are crawled, but then the crawling also broken out and executed across multiple parts of the chains, which is super difficult to track. this should probably be ripped out and simplified to more easily see when files are coming in and being farmed out to command chains.

long story short, maybe test dropping that code back in and see what happens?

@jordanpadams
Copy link
Member

@al-niessner ☝️ in case you are interested

@jordanpadams
Copy link
Member

jordanpadams commented Nov 19, 2022

I have my moments @viviant100 ☺️

@nutjob4life as a friend of mine likes to say, not just a hat rack 😄

@gxtchen
Copy link

gxtchen commented Dec 29, 2022

I see the warnings back on 3.1.0-SNAPSHOT.

@jordanpadams
Copy link
Member

jordanpadams commented Dec 29, 2022

@gxtchen we will take a look.

as a note, please be sure to tag me and/or Thomas in your comments so we can track this. otherwise, I am pretty sure I will never see it

@jordanpadams jordanpadams reopened this Dec 29, 2022
@jordanpadams
Copy link
Member

actually @gxtchen are you saying this was tested successfully or unsuccessfully?

@jordanpadams jordanpadams added invalid This doesn't seem right and removed sprint-backlog labels Jan 2, 2023
@miguelp1986
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B12.1 B13.1 bug Something isn't working i&t.done i&t.issue invalid This doesn't seem right
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants