Skip to content

Conversation

timurphy
Copy link
Contributor

@timurphy timurphy commented May 1, 2017

Fixes #744. SPDX:RDF output now contains the correct license ref:
<spdx:licenseInfoInFile rdf:resource="http://spdx.org/licenses/Glide"/>

@max-wittig
Copy link
Contributor

Code looks good and seems to work, but maybe you could also change it in the Bulk Scan and User Decision Section of the License Browser, while you're at it. That would make it consistent throughout the whole project.
image
image

@max-wittig max-wittig self-requested a review May 1, 2017 08:26
Copy link
Contributor

@max-wittig max-wittig left a comment

Choose a reason for hiding this comment

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

change 3DFX globally to Glide

@mcjaeger mcjaeger self-assigned this May 5, 2017
@timurphy
Copy link
Contributor Author

timurphy commented May 8, 2017

Hi @max-wittig, this is showing up as Glide on my side:

@max-wittig
Copy link
Contributor

That is weird. Tried using another browser and clearing the caches. Still Glide doesn't seem to be in the list for bulk scan and user decision. 3DFX still is though.
I just ran sudo make install and also tried running the fo-postinstall script again. Did I forget something?

image

@timurphy
Copy link
Contributor Author

timurphy commented May 9, 2017

Are you running this as a fresh install? I'm assuming install/db/licenseref.sql is run as part of the initial setup and (also assuming) bulk scan and user decision pull these tags from the database.

If you run a SELECT rf_shortname FROM license_ref WHERE rf_pk=199, does it give you 3DFX or Glide?

Copy link
Contributor

@max-wittig max-wittig left a comment

Choose a reason for hiding this comment

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

Works after running fo-cleanold --delete-everything and fresh reinstall

@shaheemazmalmmd
Copy link
Member

Changes will create duplicate license for existing fossology instances.

I think the update query fails.

3dfxandglide

@mcjaeger
Copy link
Member

actually, if we cannot upgrade existing instances, this is a no go then.

Copy link
Contributor

@max-wittig max-wittig left a comment

Choose a reason for hiding this comment

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

Doesn't work for existing instances of fossology

@timurphy
Copy link
Contributor Author

I think I've found the problem. fossinit.php assumes rf_shortname is the identifying key for a license, so changing this value results in a duplicate record being created:

$sql .= " where rf_shortname = '$escaped_name'";

This entire section ignores rf_pk, including when inserting new licenses. This is the true primary key and should be used as such.

It will take a couple of weeks for me to test this properly. Watch this space.

@timurphy timurphy force-pushed the 3dfx_glide-mur399 branch from 66f4c0d to 6a86482 Compare May 22, 2017 07:00
@timurphy
Copy link
Contributor Author

timurphy commented May 22, 2017

The latest commit resolves the licenseref updating issue. I've done a bit of refactoring here as there were a few issues with the current implementation:

  1. Use rf_pk as the primary key instead of rf_shortname, which was partially to blame for the duplication issue mentioned above.
  2. License update/insert queries were using hard-coded field names, with some fields missing (e.g. rf_pk). This meant that some fields were not updated. Changed to dynamically generate the query based on the actual fields in the table.
  3. License update/insert queries were open to SQL injection vulnerabilities. Changed to use parameterised queries.
  4. DbManager did not allow queryOnce to take a parameter list. Added this option.

I've run a number of tests using both blocks of modified code (update and insertion of licenses), and this appears to be working well. I would welcome some extra testing though.

Note that the changes to the Sqlite query method could not be tested, as I can't see how to configure FOSSology to use this DBMS. If someone could test this or point me in the right direction, that would be appreciated.

Copy link
Contributor

@max-wittig max-wittig left a comment

Choose a reason for hiding this comment

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

Looks good to me

@mcjaeger mcjaeger self-assigned this May 23, 2017
@mcjaeger
Copy link
Member

I tested it stand alone / fresh install -> OK, but I think we need to test an update of an existing instance.

@steffen-weber
Copy link
Contributor

I suggest testing with an >=30d old existing instance, i.e., before rf_pk=571, rf_name='RSA-Cryptoki' was inserted. Then add a new license that probably get automatically rf_pk=571. Now updating the instance and check if the new license still exists.

@timurphy
Copy link
Contributor Author

@steffen-weber this will cause the newly-created license to be overwritten, as it will have the same rf_pk as RSA-Cryptoki. Which is not good.

I'm not sure if this is fixable given the current design. Although rf_pk is set in licenseref.sql, it's not actually used as the license identifier - rf_shortname seems to have this role. This means that rf_pk may be different across installations for the same license, and also means rf_shortname cannot reliably be modified by fossinit.php. I can't think of a reliable way of fixing this without breaking existing installations.

@steffen-weber
Copy link
Contributor

Overwriting user's licenses sounds bad. How about this strategy?

  • insert Glide as duplicated license as your first commit did via initLicenseRefTable()
  • update the 3DFX text, name
  • delete newly inserted Glide license
  • update 3DFX shortname to Glide
  • increase $sysconfig['Release']

The $sysconfig['Release'] in fossinit.php should be useful to determine if migration step is required.

@timurphy
Copy link
Contributor Author

That could work. Do we have a patches.php script or similar which we could put this in? Adding this logic to fossinit.php could make that script messy in the future.

@mcjaeger
Copy link
Member

@timurphy no problem with keeping this PR open, but let us know if you feel like you are going ahead with this or not. I think changing the SPDX conformant Id to glide is an important fix.

@timurphy
Copy link
Contributor Author

@mcjaeger sorry, I haven't had time to make the changes. I'll try to get a PR in for this in the next few weeks.

@shaheemazmalmmd
Copy link
Member

@timurphy : Can you please rebase this branch with master (merge conflicts)

@timurphy timurphy force-pushed the 3dfx_glide-mur399 branch from 8510e2e to 2e3fde2 Compare July 24, 2017 06:08
@timurphy timurphy force-pushed the 3dfx_glide-mur399 branch from c8d4474 to 40775c5 Compare July 24, 2017 06:24
@timurphy
Copy link
Contributor Author

I've made the changes and tested on a fresh install, upgrade on system having rf_shortname '3DFX' and upgrade on system having rf_shortname 'Glide'. I'm not familiar with the way $sysconfig['Release'] is used so please review and let me know if I've done that wrong.

@steffen-weber
Copy link
Contributor

code looks good to me

@shaheemazmalmmd shaheemazmalmmd merged commit 25da4d3 into fossology:master Jul 24, 2017
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.

5 participants