Skip to content

Features/add-remote-reference-support #1

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

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

p1-ra
Copy link
Owner

@p1-ra p1-ra commented Feb 5, 2021

TBD

@p1-ra p1-ra self-assigned this Feb 5, 2021
@p1-ra p1-ra force-pushed the features/add-remote-reference-support branch 2 times, most recently from 1b29991 to 0eb0fc0 Compare February 5, 2021 23:54
@p1-ra p1-ra force-pushed the features/add-remote-reference-support branch 2 times, most recently from 0829d7f to 4d1804d Compare February 12, 2021 17:47
@p1-ra p1-ra force-pushed the features/add-remote-reference-support branch from 4d1804d to 33276c9 Compare February 12, 2021 18:26
@p1-ra p1-ra force-pushed the features/add-remote-reference-support branch from 1cb0ddb to 1805b0d Compare February 14, 2021 21:38
Copy link
Owner Author

@p1-ra p1-ra left a comment

Choose a reason for hiding this comment

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

You can try to parse https://github.com/raw/jdegre/5GC_APIs/master/TS29504_Nudr_DataRepository.yaml

It ~succeed before that I made some breaking changes on other part of code. To see if there the change broke the ResolvedScehama or not:

  • Finally document was not a valid oai one due to the final merge (#/components/securitySchemes) was incorrect
  • There was one external reference that failed to be resolved

from .pointer import Pointer


class Reference:
Copy link
Owner Author

Choose a reason for hiding this comment

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

I made some refactoring on the Reference object, that as has been quickly integrated to the ResolvedSchema, but not tested, there is maybe some thing to fix there

def _process(self) -> None:
self._process_remote_paths()
self._process_remote_components(self._root)
self._root.update(self._resolved_remotes_components)
Copy link
Owner Author

Choose a reason for hiding this comment

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

The merge here is too wild, it need fixe. I was doing my test on: https://github.com/raw/jdegre/5GC_APIs/master/TS29504_Nudr_DataRepository.yaml

It's #components/securitySchemes was erased by the ones of _resolved_remotes_components, leading to an invalid document. Also potentially some naming collision to handle regarding the resolved remote reference and the existing one on the root schema

return
else:
pass
# print('=' * 120)
Copy link
Owner Author

Choose a reason for hiding this comment

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

Here we have a collision to handle, we have two remote components with the same name but different content, may be implemented later, but definitly required

Copy link
Owner Author

Choose a reason for hiding this comment

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

Also, we should probably extract the code responsible to detect collisions into its own class, thus later on we could customise collision detection with DI.
Assigned new reference name should also be handle there (Same goal, for easy behaviour customization)

class ResolvedSchema:
def __init__(self, root: SchemaData, refs: Dict[str, SchemaData], errors: List[str]):
self._root: SchemaData = root
self._refs: Dict[str, SchemaData] = refs
Copy link
Owner Author

@p1-ra p1-ra Feb 24, 2021

Choose a reason for hiding this comment

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

In last refactor, the keys of refenrences dict changed from:
./TSXXXX, to /absolute/path/to/TSXXX, in order to avoid the case where two external ref have a same relative reference to them pointing to two different files.

It may need some fix in ResolvedSchema class, not tested, not sure if that change introduced behaviour change or not

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.

2 participants