Dependabot core issue npe composer fileparser

[GarryHurleyJr]

The solution was more elegant when simplified.

[honeyankit]

Could we write context and what approach did you take to fix it?

[GarryHurleyJr]

Could we write context and what approach did you take to fix it?

Sorry. It looks as if I was overanalyzing what could be null and not what was null. i was trying to be overly paranoid and raise an exception. WHat happened was that the results of the JSON parse were coming up with an empty set of dependencies.

parsed_lockfile.fetch(key, []).find { |d| d["name"] == name } was generating an NPE. My analysis was that key, parsed_lockfile, and the results of parsed_lockfile.fetch(key,[]) could be null. In some cases, the latter is the case. However, if the results of the fetch come back as an empty set, you cannot do a find call when there is nothing to search, so this could also result in an NPE. I added a qualifier to only check if the result of the fetch was not empty.
parsed_lockfile.fetch(key, []).find { |d| d["name"] == name } if !parsed_lockfile.fetch(key, []).empty?

Since lint had an issue with the 'if !..." syntax, I changed it to 'unless' but the logic is the same.

[abdulapopoola]

Please is it possible to have tests to cover this scenario?

Adding @bdragon for reviews too

[GarryHurleyJr]

Please is it possible to have tests to cover this scenario?

Adding @bdragon for reviews too

The existing test cases were what exposed the issue. If a parsed_lockfile.fetch(key,[]) returns an empty array, that would not necessarily be an error condition. However, treating it as though the array contained elements would search the empty array for a dependency, d, and would result in the NPE. On thinking about the code, I should probably also check to see if the parsed_lockfile.fetch(key,[]) is nil as well as checking for it to be empty, just for sanity.That would make the code a bit more complex to read and I just want to set up a variable to save the typing/reading/processing of parsed_lockfile.fetch(key,[]) multiple times. My rule of thumb is "Once is nice, moreso than twice, but never thrice".

[abdulapopoola]

This issue was raised here; the existing test cases did not catch it. How do we prevent a regression if we have no test coverage?

[GarryHurleyJr]

This issue was raised here; the existing test cases did not catch it. How do we prevent a regression if we have no test coverage?

there is now a test case to cover the specific cause for this failure, but the fix should prevent future recurrences in composer. I am scanning through the codebase to see if other sections can beneefit from similar changes.

[bdragon]

The second argument to Hash#fetch will be returned if key is not in hash, but you're right that if the key is present but the value is nil, the result will be nil.

The fetchresults.empty? check is unnecessary because calling #find on an empty array is fine and will just return nil.

Here's another version to consider. Here I'm using the safe navigation operator to chain method calls that might return nil.

      def lockfile_details(name:, type:)
        return unless parsed_lockfile

        key = lockfile_key(type)
        parsed_lockfile.fetch(key, [])&.find { |d| d["name"] == name }
      end

[GarryHurleyJr]

I wanted to err on the side of readability and while the empty array should return nil, I just don't trust running find on an empty array. I know it likely is not a problem in Ruby, and probably won't be in the foreseeable future, but I have just been caught flatfooted by changes in languages that I thought would never happen. This way, we are sure we won't get that error if someone changes the way find is implemented in the future. I can simplify as you put it, however, since it is cleaner and does the same.

The scenario was where the lock file had a line that said "packages-dev:null". That threw an NPE because while the key was there, the value was null.

[bdragon]

Would you mind adding a link to the bug/issue this is addressing somewhere? I understand what this test is doing but I'm not sure what behavior it's trying to fix/verify.

[GarryHurleyJr]

Hey Bryan,
The issue is #5919

[bdragon]

Thanks!