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

Fix Issue #323 #325

Closed
wants to merge 1 commit into from
Closed

Fix Issue #323 #325

wants to merge 1 commit into from

Conversation

abhi-kr-2100
Copy link

The example RDF parser can't parse the example spec from the spdx-spec repo.

This PR is trying to fix it.

If `doc.ext_document_references` is empty, an empty
`ExternalDocumentRef` must be added to it before trying to operate on
it.

Signed-off-by: Abhishek Kumar <abhi.kr.2100@gmail.com>
@abhi-kr-2100
Copy link
Author

Please note that even after resolving the exception mentioned in Issue #323, the RDF parser still fails due to other bugs. This PR is hence still in progress.

I opened it to receive feedback on whether the direction I'm taking while trying to debug is acceptable; I'm new to SPDX and may have missed something. Please let me know.

@meretp
Copy link
Collaborator

meretp commented Nov 29, 2022

Hi @abhi-kr-2100! Thanks for your contribution!
Unfortunately, I see two problems here:

  • Your current solution would possibly overwrite existing information. For example, if one complete externalDocumentRef (with ID) has already been parsed and the second externalDocumentRef misses an ID (as in the example file) the checksum and spdx_document_uri fields of the first externalDocumentRef would be overwritten with the values from the second externalDocumentRef.
  • That said, this issue could be an issue with the provided example file. I opened an issue concering the missing node for the ExternalDocumentID in the spdx-spec. The current implementation expects to find an externalDocumentID-node (see here) which would then mark the beginning of a new externalDocumentRef. As this node is missing in the example file the parser fails when setting the other attributes. Assuming that the example is wrong, you would have to include a check beforehand so that the parser raises an exception if the externalDocumentID is not found and terminates and does not raise an IndexError when trying to set the other values. But before you go any further in this, I would recommend to wait for an answer in the open issue in the spec.

@nicoweidner
Copy link
Collaborator

Hi @abhi-kr-2100 ! Thanks for the contribution!
In addition to what @meretp said, just some general words of advice: We are planning to do the next release very soon, and had already decided to postpone rdf support, since there are too many open issues (see e.g. #295 and #227 (comment)). After the release, there will be a major break; large parts of the library have to be rewritten to be more modular and maintainable.
What this means: If you find small rdf issues to fix like this one, that's great! But better hold off on any rdf issues that take longer than, say, a few days.

@abhi-kr-2100
Copy link
Author

@meretp @nicoweidner Thank you for the advice and pointers. As a beginner to open source contributions, I really appreciate it. I'll be looking around and see what I can meaningfully contribute to. :)

@nicoweidner
Copy link
Collaborator

@abhi-kr-2100 If you want to get involved in where development is going currently, you're welcome to join our weekly call. It's tomorrow, Thursday, 5pm - 5.30pm GMT, https://zoom.us/j/98741582779. The regular time is half an hour earlier, but the first Thursday every month is shifted because of a conflict. It's open to everyone, and meant as an informal way to sync on current topics and where development should focus next.

I hope the current transitional phase will be over soon. Once we have the new data model merged (currently on branch https://github.com/spdx/tools-python/tree/refactor-python-tools), there will be plenty of areas to get involved in - for example, porting the writers over to the new data model.

@abhi-kr-2100
Copy link
Author

@nicoweidner That sounds awesome. I'm looking forward to it. Thanks once again!

@abhi-kr-2100 abhi-kr-2100 closed this by deleting the head repository Jan 12, 2023
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.

3 participants