Object wrappers for OBS XML #1349

dmach · 2023-07-04T11:51:33Z

No description provided.

osc/obs_api/configuration.py

osc/_private/api.py

test_obs_api_project.py

osc/obs_api/xmlmodel/fields.py

osc/obs_api/project.py

dmach · 2023-07-04T12:12:14Z

Hi,
I'd like to get your feedback on this change.

What's going on:

wrapping any OBS XML into objects that are generated from OBS RelaxNG schemas
- I discussed state of the schemas with @adrianschroeter and @dirkmueller, and our conclusion was that we want rng as the source of truth
- spme xsd schemas need to be migrated to rng
- some schemas are missing
- some schemas are a bit off
- ... but all that can be fixed
osc.core (and other osc code) should not work with XML directly but use the new objects internally. The old code should go away eventually.
it should replace any custom OBS XML handling code outside osc
⚠️ the code is still under development
ℹ️ check test_obs_api_*.py for examples

@lethliel @marcus-h @adrianschroeter @dirkmueller
@crazyscientist - https://github.com/SUSE/osc-tiny
@dcermak - https://github.com/dcermak/py-obs (and possibly also other projects)
@Vogtinator @nilxam - https://github.com/openSUSE/openSUSE-release-tools
@mimi1vx - https://github.com/openSUSE/osc-plugin-qam
@SchoolGuy - https://github.com/openSUSE/sle-prjmgr-tools

What do you think?
Do you consider this a good idea?
If you have any concerns, suggestions of feature requests, please let me know.

Vogtinator · 2023-07-04T12:37:38Z

Sounds like a good idea.

it should replace any custom OBS XML handling code outside osc

How are the wrappers generated? Are there wrappers for XML documents not used by osc as well?

dmach · 2023-07-04T12:46:04Z

Sounds like a good idea.

it should replace any custom OBS XML handling code outside osc
How are the wrappers generated?

I have couple python scripts doing that. Extremely ugly code at the moment, but I'm planning to include it in this pull request once I clean it up.

Are there wrappers for XML documents not used by osc as well?

Not yet, because I wanted to start with the document used by osc.
Throwing anything else in is possible, but I'll do it upon request.
I just don't want to introduce API nobody uses...

crazyscientist · 2023-07-05T12:06:41Z

What do you think? Do you consider this a good idea?

From the viewpoint of a developer, who needs to handle the XML directly (either to prepare a request or process a response), I certainly would consider this a good idea: Having Python objects means that the IDE can provide helpful information, which the dev does not need to look up manually. 👍

wrapping any OBS XML into objects that are generated from OBS RelaxNG schemas

"Unfortunately", I decided to use lxml for osc-tiny and let the invoking code deal with objects generated by lxml.objectify.

By the way, did you use a tool like "generateDS"?

mimi1vx · 2023-07-05T12:10:35Z

osc/obs_api/choices.py

Shouldn't be here Enum's ?

mimi1vx · 2023-07-05T12:13:20Z

osc/obs_api/xmlmodel/fields.py

+from .xml import ET
+
+
+class Field(property):


Abstract Class?

mimi1vx · 2023-07-05T12:18:08Z

I think we talked about it on OSC23 :D good thing. In the worst case, I modernize my plugin

osc/obs_api/choices.py

osc/obs_api/xmlmodel/fields.py

dcermak

I really like this! This new API would make py-obs more or less completely obsolete (also, the API is more or less the same 😉), except for the asyncio support, but I could live without that.

dmach · 2023-07-07T07:23:38Z

"Unfortunately", I decided to use lxml for osc-tiny and let the invoking code deal with objects generated by lxml.objectify.

lxml.objectify is great when you want to avoid dealing with XML directly, but I wanted something more robust.
Also I wanted to generate the classes so I can generate docs from them.

By the way, did you use a tool like "generateDS"?

I was looking for existing tools but I missed this one.
I ended up writing my own in the end (to be published after a massive code cleanup).

SchoolGuy · 2023-07-07T11:44:20Z

I think @gyr is the better person to ask but in any case here are my two cents:

I think the big advantage is that if we are using auto-generated code changes in the XML will now cause compile-time/linter-time issues which would cause us to see breakage before it happens rather than during runtime when the XML is being parsed.

For sure you can make the XML handling code robust enough to consider that the schema has changed but it still doesn't tell you that something is wrong before it is executed.

I am overall for this change. The recursive imports are worrying me a bit but I believe that this is a WIP PR and as such this can be addressed if required at a later point. From my side please go forward with this! Maybe at some point, we can go all the way to protobuf and gRPC to not maintain our own scripting for generating the code for this.

pep8speaks · 2024-02-12T14:16:32Z

Hello @dmach! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file osc/util/models.py:

Line 710:121: E501 line too long (159 > 120 characters)

Comment last updated at 2024-02-21 08:47:16 UTC

codecov · 2024-02-12T14:17:16Z

Codecov Report

Attention: 398 lines in your changes are missing coverage. Please review.

Comparison is base (cff6a0c) 31.51% compared to head (b03f5d3) 31.43%.
Report is 8 commits behind head on master.

Files	Patch %	Lines
osc/util/models.py	62.65%	90 Missing ⚠️
osc/obs_api/enums.py	0.00%	67 Missing ⚠️
osc/obs_api/package.py	0.00%	46 Missing ⚠️
osc/obs_api/project.py	0.00%	44 Missing ⚠️
osc/obs_api/repository.py	0.00%	20 Missing ⚠️
osc/obs_api/repository_download.py	0.00%	17 Missing ⚠️
osc/obs_api/simple_flag.py	0.00%	14 Missing ⚠️
osc/core.py	0.00%	12 Missing ⚠️
osc/obs_api/flag.py	0.00%	11 Missing ⚠️
osc/obs_api/status_data.py	0.00%	11 Missing ⚠️
... and 12 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1349      +/-   ##
==========================================
- Coverage   31.51%   31.43%   -0.09%     
==========================================
  Files          51       71      +20     
  Lines       17938    18465     +527     
==========================================
+ Hits         5653     5804     +151     
- Misses      12285    12661     +376

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

osc/util/models.py

 import types
+import typing


osc/util/models.py

+
+    @classmethod
+    def xml_request(cls, method: str, apiurl: str, path: List[str], query: Optional[dict] = None, data: Optional[str] = None) -> urllib3.response.HTTPResponse:
+        from ..connection import http_request


osc/util/models.py

+    @classmethod
+    def xml_request(cls, method: str, apiurl: str, path: List[str], query: Optional[dict] = None, data: Optional[str] = None) -> urllib3.response.HTTPResponse:
+        from ..connection import http_request
+        from ..core import makeurl


osc/util/models.py

+
+        IMPORTANT: This method is always interactive.
+        """
+        from ..core import run_editor


osc/util/models.py

dmach · 2024-02-13T13:33:27Z

So, this is the second (public) iteration of the object wrapper for OBS XML.
It is relatively close to pydantic which gives us a slight chance to move from custom BaseModel and Field objects to pydantic someday. I decided to avoid pydantic for now because v2 not widespread yet and there are differences between v1 and v2. Also, the latest version doesn't work on Python 3.6 which is a requirement for osc.

osc/util/models.py

dcermak · 2024-02-19T13:03:42Z

osc/util/models.py

+                    edited_obj = self.__class__.from_file(f.name)
+                    f.seek(0)
+                    edited_data = f.read()


Shouldn't we truncate here too?

No, because we truncate on write() while this performs read()

…s in BaseModel

github-advanced-security bot found potential problems Jul 4, 2023

View reviewed changes

mimi1vx reviewed Jul 5, 2023

View reviewed changes

osc/obs_api/choices.py Outdated

Copy link

Member

mimi1vx Jul 5, 2023

Choose a reason for hiding this comment

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

Shouldn't be here Enum's ?

mimi1vx reviewed Jul 5, 2023

View reviewed changes

osc/obs_api/xmlmodel/fields.py Outdated

from .xml import ET

class Field(property):

Copy link

Member

mimi1vx Jul 5, 2023

Choose a reason for hiding this comment

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

Abstract Class?

dcermak reviewed Jul 6, 2023

View reviewed changes

osc/obs_api/choices.py Outdated Show resolved Hide resolved

dcermak reviewed Jul 6, 2023

View reviewed changes

osc/obs_api/xmlmodel/fields.py Outdated Show resolved Hide resolved

dcermak reviewed Jul 6, 2023

View reviewed changes

dmach force-pushed the xmlmodel branch from 1674c7e to 82619b2 Compare February 12, 2024 14:16

github-advanced-security bot found potential problems Feb 12, 2024

View reviewed changes

dmach force-pushed the xmlmodel branch from 82619b2 to a22bf36 Compare February 13, 2024 11:40

github-advanced-security bot found potential problems Feb 13, 2024

View reviewed changes

osc/util/models.py Dismissed Show dismissed Hide dismissed

dmach force-pushed the xmlmodel branch 3 times, most recently from 3a6af40 to 8361cf9 Compare February 13, 2024 13:02

dmach marked this pull request as ready for review February 13, 2024 13:06

dmach requested review from dcermak and mimi1vx February 13, 2024 13:06

dcermak reviewed Feb 19, 2024

View reviewed changes

osc/util/models.py Outdated Show resolved Hide resolved

dcermak reviewed Feb 19, 2024

View reviewed changes

dmach added 2 commits February 21, 2024 09:46

Add XmlModel class that encapsulates manipulation with XML

e5774a5

Add XML models for Project and Package

0dd1f52

dmach added 3 commits February 21, 2024 09:46

Migrate core.show_devel_project() to obs_api.Package

00d2192

Implement do_snapshot() and has_changed() methods to determine change…

ab36108

…s in BaseModel

Migrate core.set_devel_project() to obs_api.Package

b03f5d3

dmach force-pushed the xmlmodel branch from 8361cf9 to b03f5d3 Compare February 21, 2024 08:46

dmach merged commit af24f3c into openSUSE:master Feb 23, 2024
31 of 35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Object wrappers for OBS XML #1349

Object wrappers for OBS XML #1349

dmach commented Jul 4, 2023

dmach commented Jul 4, 2023

Vogtinator commented Jul 4, 2023

dmach commented Jul 4, 2023 •

edited

Loading

crazyscientist commented Jul 5, 2023

mimi1vx Jul 5, 2023

mimi1vx Jul 5, 2023

mimi1vx commented Jul 5, 2023

dcermak left a comment

dmach commented Jul 7, 2023

SchoolGuy commented Jul 7, 2023

pep8speaks commented Feb 12, 2024 •

edited

Loading

codecov bot commented Feb 12, 2024 •

edited

Loading

dmach commented Feb 13, 2024 •

edited

Loading

dcermak Feb 19, 2024

dmach Feb 19, 2024

Object wrappers for OBS XML #1349

Object wrappers for OBS XML #1349

Conversation

dmach commented Jul 4, 2023

dmach commented Jul 4, 2023

Vogtinator commented Jul 4, 2023

dmach commented Jul 4, 2023 • edited Loading

crazyscientist commented Jul 5, 2023

mimi1vx Jul 5, 2023

Choose a reason for hiding this comment

mimi1vx Jul 5, 2023

Choose a reason for hiding this comment

mimi1vx commented Jul 5, 2023

dcermak left a comment

Choose a reason for hiding this comment

dmach commented Jul 7, 2023

SchoolGuy commented Jul 7, 2023

pep8speaks commented Feb 12, 2024 • edited Loading

Comment last updated at 2024-02-21 08:47:16 UTC

codecov bot commented Feb 12, 2024 • edited Loading

Codecov Report

dmach commented Feb 13, 2024 • edited Loading

dcermak Feb 19, 2024

Choose a reason for hiding this comment

dmach Feb 19, 2024

Choose a reason for hiding this comment

dmach commented Jul 4, 2023 •

edited

Loading

pep8speaks commented Feb 12, 2024 •

edited

Loading

codecov bot commented Feb 12, 2024 •

edited

Loading

dmach commented Feb 13, 2024 •

edited

Loading