Opened 2 years ago

Closed 18 months ago

#22695 closed defect (fixed)

Resolve ambiguity between descriptorFile and fileName when parsing descriptors

Reported by: iwakeh Owned by: iwakeh
Priority: Medium Milestone: metrics-lib 2.3.0
Component: Metrics/Library Version:
Severity: Normal Keywords: metrics-2018
Cc: metrics-team Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by iwakeh)

In 1.9.0 a new method getDescrptorFile was introduced. This actually can cause various problems and has some open design questions that should be answered in this ticket.
(I'll attach a branch with a test demonstrating part of the problem.)

DescriptorParser has the new method (since 1.9.0):

public Iterable<Descriptor> parseDescriptors(byte[] rawDescriptorBytes,
    File descriptorFile, String fileName);

In addition to the problem demonstrated in the (to be attached) test this raises some questions:

1) What should be done, if descriptorFile.getName() != fileName?
2) What should be done, if descriptorFile != null && (null == fileName || fileName.isEmpty())?
3) What should be done, if descriptorFile == null && null != fileName?
4a) What should be done, if descriptorFile == null && null != fileName && fileName.isEmpty()?
4b) What should be done, if descriptorFile == null && null == fileName?

The implementation of the solution should also have tests for each of the possible cases.

Child Tickets

Change History (16)

comment:1 Changed 2 years ago by iwakeh

The failing assertion is assertNotNull(desc.getDescriptorFile()); in this branch.

comment:2 Changed 2 years ago by iwakeh

First thoughts about possible answers:

1) Several answers:

i) some form of exception?
ii) warning and use the descriptorFile given?

2) Set fileName = descriptorFile.getName().
3) More tricky:

i) use descriptorFile = new File(fileName) is potentially problematic b/c there is no such file.
ii) leave descriptorFile = null and forget about the given filename?
iii) add getDescriptorFilename() to the Descriptor interface?
iv) some form of exception?

4) some form of exception?

comment:3 Changed 2 years ago by iwakeh

Description: modified (diff)

comment:4 Changed 2 years ago by karsten

I don't have a good answer yet, but my hope is that we can get rid of fileName in the medium term. See also #22207. Maybe there are more descriptors that we'd have to change for this, but I really hope that all descriptors will be self-describing without relying on file names in the future.

Regarding descriptorFile, I think we'll have to permit null there, because it's not a strict requirements that descriptors come from a descriptor file. For example, an application might want to store descriptors in a database and parse them at a later time, and then it won't necessarily have the file name anymore.

I'll think more about this and maybe come up with a better answer later.

comment:5 Changed 2 years ago by karsten

I'm not sure whether we need to do anything at all here. The JavaDocs specify what the two parameters are used for:

   * @param descriptorFile Optional descriptor file reference included in
   *     parsed/unparseable descriptors
   * @param fileName Descriptor file name used for parsing the descriptor
   *     publication time of some descriptor types

Basically, there is no formal relationship between the two parameters. In many cases fileName will be the same as descriptorFile.getName(), but that's not required. The two parameters are used for very different purposes.

And ideally we'd get rid of fileName anyway and only keep the optional descriptorFile. But that's for later.

So, I don't see a bug here (yet). I do see an interface that could be designed more clearly. But I think the best way to get there is to throw out fileName as soon as we can.

What do you think needs to be done here?

comment:6 Changed 2 years ago by iwakeh

Why not throw out the File parameter?
If byte arrays are supplied the filename parameter can be used for adding the 'source' to the descriptor raw bytes, which may be useful for debugging/troubleshooting and identifying weird descriptors.
Or, turn filename into a sourceMarker string?

comment:7 Changed 2 years ago by iwakeh

Milestone: metrics-lib 2.0.0

Removing this from 2.0.0, but ambiguous design is a defect.
The interdependence of the two parameters is a very valid assumption.

comment:8 in reply to:  6 Changed 2 years ago by karsten

Replying to iwakeh:

Why not throw out the File parameter?

We want the File reference to be set in every Descriptor that DescriptorParser returns. We could indeed throw out the File parameter and set it ourselves.

If byte arrays are supplied the filename parameter can be used for adding the 'source' to the descriptor raw bytes, which may be useful for debugging/troubleshooting and identifying weird descriptors.

I'd rather get rid of it.

Or, turn filename into a sourceMarker string?

The issue is that we sometimes parse parts from the filename and include it in the Descriptor. I want to change that. But not before 2.0.0 comes out. :)

comment:9 in reply to:  7 Changed 2 years ago by karsten

Replying to iwakeh:

Removing this from 2.0.0, but ambiguous design is a defect.
The interdependence of the two parameters is a very valid assumption.

I agree. I also feel we're not done yet with making interfaces self-explanatory or even unambiguous. But I also agree that it's something for post-2.0.0.

comment:10 Changed 2 years ago by karsten

Summary: descriptor file processingResolve ambiguity between descriptorFile and fileName when parsing descriptors

Attempt to make summary clearer.

comment:11 Changed 2 years ago by karsten

Keywords: metrics-2018 added

comment:12 Changed 20 months ago by karsten

Priority: HighMedium

This ticket has not been modified in the last 2 months or even longer. Setting priority to medium.

comment:13 Changed 18 months ago by iwakeh

Owner: changed from metrics-team to iwakeh
Status: newaccepted

comment:14 Changed 18 months ago by iwakeh

Cc: metrics-team added
Status: acceptedneeds_review

Please review this patch, which (hopefully) clarifies the situation by a more verbose javadoc description and a parameter name change.

comment:15 Changed 18 months ago by iwakeh

Milestone: metrics-lib 2.3.0

comment:16 Changed 18 months ago by karsten

Resolution: fixed
Status: needs_reviewclosed

Patch looks good! Merged. Closing. Thanks!

Note: See TracTickets for help on using tickets.