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

add an ExtractedRecord object that cannot have extracted records #896

Closed

Conversation

dev-mlb
Copy link
Collaborator

@dev-mlb dev-mlb commented Aug 26, 2024

No description provided.

@jpdahlke jpdahlke changed the title ExtractedRecord object that cannot have extracted records add an ExtractedRecord object that cannot have extracted records Aug 27, 2024
@dev-mlb dev-mlb changed the title add an ExtractedRecord object that cannot have extracted records extracted records cannot have extracted records - option 1 Aug 27, 2024
@drivenflywheel
Copy link
Collaborator

drivenflywheel commented Aug 27, 2024

I like this approach better than the "Option 2" approach. Honestly, I'd love to decompose the IBDO mega-interface, having it extend a simpler interface (IBaseRecord?) that doesn't have any of the content components - no data(), setData() channelFactory, newInputStream(), header(), footer(), size(), dataLength() ... IBDO would add those on top of the new interface, ExtractedRecord would implement that new interface, and (eventually?), the ExtractedRecords collection would be a collection of IBaseRecord or ExtractedRecord, not a collection of IBDO.

Basically, it would be nice if ExtractedRecord didn't have to override a lot of methods just to say it doesn't support those methods. Would be better if it just didn't have them to reference.

@dev-mlb dev-mlb changed the title extracted records cannot have extracted records - option 1 extracted records cannot have extracted records - option 1 add ExtractedRecord object Aug 27, 2024
@jdcove2
Copy link
Collaborator

jdcove2 commented Aug 29, 2024

Given the title of this PR as the objective, then another solution would be to break the IBaseDataObject into two interfaces. The parent interface (IExtractedRecord) would contain everything in the IBaseDataObject except the extracted record methods. The child interface (IBaseDataObject) would extend the parent and add the extracted record methods which would now take an IExtractedRecord instead of an IBaseDataObject. BaseDataObject (and the new ExtractedRecord) would mirror the interfaces. All of the code would continue to create BaseDataObject implementations except when code wants to add an IExtractedRecord to an IBaseDataObject where it would need to change from creating BaseDataObject instances to ExtractedRecord instances.

@jdcove2
Copy link
Collaborator

jdcove2 commented Aug 30, 2024

Additionally, the transition can by non-breaking by initially leaving the extracted record methods using IBaseDataObject, transition all of the downstream code from IBaseDataObject to IExtractedRecord and once all the downstream code has been transitioned then the extracted record methods can be changed use IExtractedRecord.

@jpdahlke
Copy link
Collaborator

jpdahlke commented Sep 1, 2024

I like this approach better than the "Option 2" approach. Honestly, I'd love to decompose the IBDO mega-interface, having it extend a simpler interface (IBaseRecord?) that doesn't have any of the content components - no data(), setData() channelFactory, newInputStream(), header(), footer(), size(), dataLength() ... IBDO would add those on top of the new interface, ExtractedRecord would implement that new interface, and (eventually?), the ExtractedRecords collection would be a collection of IBaseRecord or ExtractedRecord, not a collection of IBDO.

Basically, it would be nice if ExtractedRecord didn't have to override a lot of methods just to say it doesn't support those methods. Would be better if it just didn't have them to reference.

I like where this idea is going I think. I wanted to lay something like this out visually with some other thoughts/adjustments.

IBaseRecord (many things would just move up to this level withholding some of the data manipulation methods described above)
 -> IBaseDataObject (allows for manipulation of data and extracted records to be added like today)
   -> TopLevelObject (FUTURE: could anchor all trees, replaces need for isTld check in some places)
   -> BaseDataObject (a child object like today)
 -> IExtractedRecord (does not allow for manipulation of data once created)
   -> ExtractedRecord

All that said, this PR seems to give us the immediate need of an enforcement mechanism (which was the ask) while setting the stage for doing what you suggest. I guess the question is, how immediate is the need and do we tackle this in parts with this being the first step or do we start a larger process now? I would imagine that the actual existence of an ExtractedRecord class will start buying us some benefit without too many downstream changes or the need for interface changes in the immediate. As long as we have a compatible loose plan for getting there, I don't think this implementation boxes us out.

@drivenflywheel
Copy link
Collaborator

As long as we have a compatible loose plan for getting there, I don't think this implementation boxes us out.

Agreed. I think we could move forward with this approach.

@jdcove2
Copy link
Collaborator

jdcove2 commented Sep 4, 2024

I submitted draft Emissary PR#911 to show the specifics of my above suggestion.

@dev-mlb
Copy link
Collaborator Author

dev-mlb commented Sep 5, 2024

@jpdahlke @drivenflywheel This is a much bigger change than I originally intended, but I added the IBaseRecord and IExtractedRecord interfaces. I created an abstract BaseRecord object that had most of the BaseDataObject code in it. For this ticket, I only kept the extracted records in the IBaseDataObject. I figured we could break this down into smaller chunks (data, channels, etc.) later on and tackle them one by one.

@dev-mlb dev-mlb added the integration A breaking change where effort will be required downstream to resolve label Sep 5, 2024
@dev-mlb dev-mlb changed the title extracted records cannot have extracted records - option 1 add ExtractedRecord object add an ExtractedRecord object that cannot have extracted records Sep 5, 2024
@jdcove2
Copy link
Collaborator

jdcove2 commented Sep 5, 2024

The problem I see with this structure is in transitioning the extracted record methods from using IBaseDataObject to using IExtractedRecord. This structure will require all of the downstream code to switch at one time. EPR#911 allows a mixed environment so that the code does not need to be transitioned at one time. If an extended transition is desired, EPR#911 could be implemented first and then when all of the downstream code is transitioned it could be restructured to look like this PR.

@jpdahlke
Copy link
Collaborator

jpdahlke commented Sep 10, 2024

I think commit 2 illustrates how easy it would be to change the interfaces around once we have an ExtractedRecord class (albeit a breaking change downstream which we wouldn't take yet). With only the first commit, we'd be subject to unsupported method exceptions, but I'm guessing the pros outweigh the cons? What do you think @drivenflywheel?

@dev-mlb
Copy link
Collaborator Author

dev-mlb commented Sep 10, 2024

Updated here #922

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration A breaking change where effort will be required downstream to resolve
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants