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

[13.0] base_rest: New api #71

Merged
merged 33 commits into from Jan 15, 2021
Merged

[13.0] base_rest: New api #71

merged 33 commits into from Jan 15, 2021

Conversation

lmignon
Copy link
Sponsor Contributor

@lmignon lmignon commented May 26, 2020

Foward port of #48

… to get the registry from the context since it's always available on the schema class
… fixes the incompatibility with the latest marshmallow version
…a property of datamodel instances as well as on marshmallow schema instances.
@HviorForgeFlow
Copy link
Member

Not sure if related with this PR but at start running an instance I've got this error:

2020-05-26 11:24:37,780 1 CRITICAL test odoo.service.server: Failed to initialize database `test`. 
Traceback (most recent call last):
  File "/opt/odoo/custom/src/odoo/odoo/service/server.py", line 1179, in preload_registries
    registry = Registry.new(dbname, update_module=update_module)
  File "/opt/odoo/custom/src/odoo/odoo/modules/registry.py", line 86, in new
    odoo.modules.load_modules(registry._db, force_demo, status, update_module)
  File "/opt/odoo/custom/src/odoo/odoo/modules/loading.py", line 535, in load_modules
    model._register_hook()
  File "/opt/odoo/auto/addons/base_rest/models/rest_service_registration.py", line 62, in _register_hook
    self._build_controllers_routes(services_registry)
  File "/opt/odoo/auto/addons/base_rest/models/rest_service_registration.py", line 66, in _build_controllers_routes
    for service in self._get_services(controller_def["collection_name"]):
  File "/opt/odoo/auto/addons/base_rest/models/rest_service_registration.py", line 98, in _get_services
    return work.many_components()
  File "/opt/odoo/auto/addons/component/core.py", line 438, in many_components
    return [comp(work_context) for comp in component_classes]
  File "/opt/odoo/auto/addons/component/core.py", line 438, in <listcomp>
    return [comp(work_context) for comp in component_classes]
  File "/opt/odoo/auto/addons/connector_search_engine/components/mapper.py", line 20, in __init__
    exporter = work.index.exporter_id
AttributeError: 'WorkContext' object has no attribute 'index'

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented May 26, 2020

@HviorForgeFlow Thank you for the report. I'll take a look ASAP.This error is strange. The mapper component should not be returned by the lookup into the component registry since it's not registered for the rest service collection..
One I've found the issue I'll come back to you

… collection

This specific lookup is required to filter out component without collection. In the same time we must also filter out components that are not services
…bers

This is required to avoid some trouble with properties resolved by the method inspect.getmembers and accessing resources not yet completly initialized
@lmignon
Copy link
Sponsor Contributor Author

lmignon commented May 26, 2020

@HviorForgeFlow The encountered issue should be resolved by the 2 last commits.

@HviorForgeFlow
Copy link
Member

It is working fine now thanks!

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented May 27, 2020

@HviorForgeFlow Thank you for the feedback. If you need to develop new services, I encourage you to use the new api and datamodels. It's much more flexible and easier to maintain.

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented May 27, 2020

@guewen @gurneyalex The new API is now also available for Odoo 13. It would be nice if you could use-it / test-it with your new WMS mobile app 😏

@gurneyalex
Copy link
Member

hello @lmignon I see #48 is flagged as "in progress". Is it also the case for #71 ?

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Aug 24, 2020

hello @lmignon I see #48 is flagged as "in progress". Is it also the case for #71 ?
Good catch @gurneyalex. In fact, both of them needs review.

@kevinkhao
Copy link
Contributor

kevinkhao commented Aug 31, 2020

@lmignon before I do a PR, I'd like to know what you think.
Here is some context: https://github.com/acsone/rest-framework/blob/37b69406818a82deedccbb0444c731cf12a2fc20/base_rest/restapi.py#L194
It's about functions from_params and to_response.
For our controllers, when we use Datamodel and because we most probably just use my_input_datamodel.dump() the on the input and my_output_datamodel.load() before returning our result:
Would it make sense to:

  1. dump our datamodel so that it is directly available in the controller's arguments ? i.e: in function from_params
    return ModelClass.load( params, many=self._is_list, unknown=marshmallow.EXCLUDE ).dump()
  2. allow to return a dict and it is automatically load()ed on the datamodel ? i.e in function to_response
    remove lines 204 to 208
    errors = ModelClass.validate(result, many=self._is_list)
    What do you think ?

Currently it does seem to me as bringing unnecessary steps, but I am maybe missing something.

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Aug 31, 2020

@kevinkhao Sorry but I don't understand your question. The @Datamodel decorator is designed to decorate methods where the result of the call to the method is an instance of Datamodel and the param is also Datamodel instance. This decorator allows to only deal with datamodel objects on the service layer implementation.

@kevinkhao
Copy link
Contributor

kevinkhao commented Oct 8, 2020

@kevinkhao Sorry but I don't understand your question. The @Datamodel decorator is designed to decorate methods where the result of the call to the method is an instance of Datamodel and the param is also Datamodel instance. This decorator allows to only deal with datamodel objects on the service layer implementation.

My idea was that when we define a route, we should just return a dict and the module will take care to do self.env.datamodel["my.output.datamodel"].load(resulting_dict) so that we can save 1 line at the end of our route definition. But after thinking about it, I'm still not sure about that idea.

Calling at the end of all my routes:
return self.env["my.datamodel"].load(myResultDict)
instead of
return myResultDict
is more verbose but more clear.

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Oct 9, 2020

My idea was that when we define a route, we should just return a dict and the module will take care to do self.env.datamodel["my.output.datamodel"].load(resulting_dict) so that we can save 1 line at the end of our route definition. But after thinking about it, I'm still not sure about that idea.

@kevinkhao But we don't have to declare a route by ourself. Routes are dynamically generated by the framework based on the @restapi.method decorator 262def2

lmignon added a commit to acsone/rest-framework that referenced this pull request Dec 24, 2020
Merge remote-tracking branch 'origin/13.0-48-port' into 14.0-migrate-base-rest-new-api
@lmignon lmignon mentioned this pull request Dec 24, 2020
2 tasks
@hparfr hparfr mentioned this pull request Jan 15, 2021
@simahawk
Copy link
Contributor

@lmignon sorry, was on holidays and missed your msg. I'll skim through your last changes in a while.

base_rest/tests/common.py Outdated Show resolved Hide resolved
@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Jan 15, 2021

@hparfr, @sebastienbeau Are you OK with this PR?

Copy link
Contributor

@hparfr hparfr left a comment

Choose a reason for hiding this comment

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

The tests I had conducted so far on WMS shows that it seams to work.
I will do more testing when porting shopinvader soon and open issues or propose PR if needed.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@simahawk
Copy link
Contributor

@lmignon good to merge then?

@lmignon
Copy link
Sponsor Contributor Author

lmignon commented Jan 15, 2021

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 13.0-ocabot-merge-pr-71-by-lmignon-bump-major, awaiting test results.

@sbidoul
Copy link
Member

sbidoul commented Jan 15, 2021

fireworks

@OCA-git-bot OCA-git-bot merged commit 45945a1 into OCA:13.0 Jan 15, 2021
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at e9f76f2. Thanks a lot for contributing to OCA. ❤️

@sebastienbeau
Copy link
Member

youhou \o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet