From 89568dc8e5e441d1cbceffe94cd39689c67f8ff5 Mon Sep 17 00:00:00 2001 From: thatmattlove Date: Fri, 24 Sep 2021 00:16:26 -0700 Subject: [PATCH] Deprecate `Device.network` --- hyperglass/configuration/main.py | 2 +- hyperglass/models/api/query.py | 6 +- hyperglass/models/api/response.py | 18 +---- hyperglass/models/config/devices.py | 30 +++++--- hyperglass/models/config/network.py | 20 ----- hyperglass/models/config/proxy.py | 2 +- hyperglass/models/tests/test_util.py | 14 +++- hyperglass/models/ui.py | 10 +-- hyperglass/models/util.py | 74 +++++++++++++++---- .../plugins/tests/test_bgp_route_juniper.py | 7 +- .../ui/components/form/queryLocation.tsx | 18 ++--- .../ui/components/results/individual.tsx | 21 +++++- hyperglass/ui/hooks/types.ts | 4 +- hyperglass/ui/hooks/useDevice.ts | 14 ++-- hyperglass/ui/hooks/useFormState.ts | 22 +++--- hyperglass/ui/types/config.ts | 48 ++---------- 16 files changed, 161 insertions(+), 149 deletions(-) delete mode 100644 hyperglass/models/config/network.py diff --git a/hyperglass/configuration/main.py b/hyperglass/configuration/main.py index 804c8db..6d1c49a 100644 --- a/hyperglass/configuration/main.py +++ b/hyperglass/configuration/main.py @@ -204,7 +204,7 @@ def init_ui_params(*, params: "Params", devices: "Devices") -> "UIParameters": return UIParameters( **_ui_params, version=__version__, - networks=devices.networks(params), + devices=devices.frontend(), parsed_data_fields=PARSED_RESPONSE_FIELDS, content={"credit": content_credit, "greeting": content_greeting}, ) diff --git a/hyperglass/models/api/query.py b/hyperglass/models/api/query.py index 0c69276..a455448 100644 --- a/hyperglass/models/api/query.py +++ b/hyperglass/models/api/query.py @@ -148,7 +148,7 @@ class Query(BaseModel): def validate_query_type(cls, value): """Ensure a requested query type exists.""" devices = use_state("devices") - if any((device.has_directives(value) for device in devices.objects)): + if any((device.has_directives(value) for device in devices)): return value raise QueryTypeNotFound(name=value) @@ -158,10 +158,8 @@ class Query(BaseModel): """Ensure query_location is defined.""" devices = use_state("devices") - valid_id = value in devices.ids - valid_hostname = value in devices.hostnames - if not any((valid_id, valid_hostname)): + if not devices.valid_id_or_name(value): raise QueryLocationNotFound(location=value) return value diff --git a/hyperglass/models/api/response.py b/hyperglass/models/api/response.py index b1d0a4d..3e6ab64 100644 --- a/hyperglass/models/api/response.py +++ b/hyperglass/models/api/response.py @@ -163,26 +163,12 @@ class Vrf(BaseModel): } -class Network(BaseModel): - """Response model for /api/devices networks.""" - - name: StrictStr - display_name: StrictStr - - class Config: - """Pydantic model configuration.""" - - title = "Network" - description = "Network/ASN attributes" - schema_extra = {"examples": [{"name": "primary", "display_name": "AS65000"}]} - - class RoutersResponse(BaseModel): """Response model for /api/devices list items.""" id: StrictStr name: StrictStr - network: StrictStr + group: StrictStr class Config: """Pydantic model configuration.""" @@ -191,7 +177,7 @@ class RoutersResponse(BaseModel): description = "Device attributes" schema_extra = { "examples": [ - {"id": "nyc_router_1", "name": "NYC Router 1", "network": "New York City, NY"} + {"id": "nyc_router_1", "name": "NYC Router 1", "group": "New York City, NY"} ] } diff --git a/hyperglass/models/config/devices.py b/hyperglass/models/config/devices.py index ab9cf43..122ae8d 100644 --- a/hyperglass/models/config/devices.py +++ b/hyperglass/models/config/devices.py @@ -27,9 +27,7 @@ from .ssl import Ssl from ..main import MultiModel, HyperglassModel, HyperglassModelWithId from ..util import check_legacy_fields from .proxy import Proxy -from .params import Params from ..fields import SupportedDriver -from .network import Network from ..directive import Directives from .credential import Credential @@ -46,7 +44,7 @@ class Device(HyperglassModelWithId, extra="allow"): id: StrictStr name: StrictStr address: Union[IPv4Address, IPv6Address, StrictStr] - network: Network + group: Optional[StrictStr] credential: Credential proxy: Optional[Proxy] display_name: Optional[StrictStr] @@ -60,7 +58,7 @@ class Device(HyperglassModelWithId, extra="allow"): def __init__(self, **kw) -> None: """Check legacy fields and ensure an `id` is set.""" - kw = check_legacy_fields("Device", **kw) + kw = check_legacy_fields(model="Device", data=kw) if "id" not in kw: kw = self._with_id(kw) super().__init__(**kw) @@ -100,7 +98,7 @@ class Device(HyperglassModelWithId, extra="allow"): return { "id": self.id, "name": self.name, - "network": self.network.display_name, + "group": self.group, } @property @@ -269,6 +267,13 @@ class Devices(MultiModel, model=Device, unique_by="id"): """Export API-facing device fields.""" return [d.export_api() for d in self] + def valid_id_or_name(self, value: str) -> bool: + """Determine if a value is a valid device name or ID.""" + for device in self: + if value == device.id or value == device.name: + return True + return False + def directive_plugins(self) -> Dict[Path, Tuple[StrictStr]]: """Get a mapping of plugin paths to associated directive IDs.""" result: Dict[Path, Set[StrictStr]] = {} @@ -286,22 +291,23 @@ class Devices(MultiModel, model=Device, unique_by="id"): # Convert the directive set to a tuple. return {k: tuple(v) for k, v in result.items()} - def networks(self, params: Params) -> List[Dict[str, Any]]: - """Group devices by network.""" - names = {device.network.display_name for device in self} + def frontend(self) -> List[Dict[str, Any]]: + """Export grouped devices for UIParameters.""" + params = use_state("params") + groups = {device.group for device in self} return [ { - "display_name": name, + "group": group, "locations": [ { "id": device.id, "name": device.name, - "network": device.network.display_name, + "group": group, "directives": [d.frontend(params) for d in device.directives], } for device in self - if device.network.display_name == name + if device.group == group ], } - for name in names + for group in groups ] diff --git a/hyperglass/models/config/network.py b/hyperglass/models/config/network.py deleted file mode 100644 index d52e5e5..0000000 --- a/hyperglass/models/config/network.py +++ /dev/null @@ -1,20 +0,0 @@ -"""Validate network configuration variables.""" - -# Third Party -from pydantic import Field, StrictStr - -# Local -from ..main import HyperglassModel - - -class Network(HyperglassModel): - """Validation Model for per-network/asn config in devices.yaml.""" - - name: StrictStr = Field( - ..., title="Network Name", description="Internal name of the device's primary network.", - ) - display_name: StrictStr = Field( - ..., - title="Network Display Name", - description="Display name of the device's primary network.", - ) diff --git a/hyperglass/models/config/proxy.py b/hyperglass/models/config/proxy.py index 4c7ddb0..638ea79 100644 --- a/hyperglass/models/config/proxy.py +++ b/hyperglass/models/config/proxy.py @@ -28,7 +28,7 @@ class Proxy(HyperglassModel): def __init__(self: "Proxy", **kwargs: Any) -> None: """Check for legacy fields.""" - kwargs = check_legacy_fields("Proxy", **kwargs) + kwargs = check_legacy_fields(model="Proxy", data=kwargs) super().__init__(**kwargs) @property diff --git a/hyperglass/models/tests/test_util.py b/hyperglass/models/tests/test_util.py index ce103c6..bb133d6 100644 --- a/hyperglass/models/tests/test_util.py +++ b/hyperglass/models/tests/test_util.py @@ -7,16 +7,24 @@ import pytest from ..util import check_legacy_fields +@pytest.mark.dependency() def test_check_legacy_fields(): test1 = {"name": "Device A", "nos": "juniper"} test1_expected = {"name": "Device A", "platform": "juniper"} test2 = {"name": "Device B", "platform": "juniper"} test3 = {"name": "Device C"} - assert set(check_legacy_fields("Device", **test1).keys()) == set( + test4 = {"name": "Device D", "network": "this is wrong"} + + assert set(check_legacy_fields(model="Device", data=test1).keys()) == set( test1_expected.keys() ), "legacy field not replaced" - assert set(check_legacy_fields("Device", **test2).keys()) == set( + + assert set(check_legacy_fields(model="Device", data=test2).keys()) == set( test2.keys() ), "new field not left unmodified" + with pytest.raises(ValueError): - check_legacy_fields("Device", **test3) + check_legacy_fields(model="Device", data=test3) + + with pytest.raises(ValueError): + check_legacy_fields(model="Device", data=test4) diff --git a/hyperglass/models/ui.py b/hyperglass/models/ui.py index d6466ce..d86e936 100644 --- a/hyperglass/models/ui.py +++ b/hyperglass/models/ui.py @@ -42,14 +42,14 @@ class UILocation(HyperglassModel): id: StrictStr name: StrictStr - network: StrictStr + group: StrictStr directives: List[UIDirective] = [] -class UINetwork(HyperglassModel): - """UI: Network.""" +class UIDevices(HyperglassModel): + """UI: Devices.""" - display_name: StrictStr + group: StrictStr locations: List[UILocation] = [] @@ -67,6 +67,6 @@ class UIParameters(ParamsPublic, HyperglassModel): web: WebPublic messages: Messages version: StrictStr - networks: List[UINetwork] = [] + devices: List[UIDevices] = [] parsed_data_fields: Tuple[StructuredDataField, ...] content: UIContent diff --git a/hyperglass/models/util.py b/hyperglass/models/util.py index 73832e5..f012906 100644 --- a/hyperglass/models/util.py +++ b/hyperglass/models/util.py @@ -3,28 +3,70 @@ # Standard Library from typing import Any, Dict, Tuple +# Third Party +from pydantic import BaseModel + # Project from hyperglass.log import log -LEGACY_FIELDS: Dict[str, Tuple[Tuple[str, str], ...]] = { - "Device": (("nos", "platform"),), - "Proxy": (("nos", "platform"),), + +class LegacyField(BaseModel): + """Define legacy fields on a per-model basis. + + When `overwrite` is `True`, the old key is replaced with the new + key. This will generally only occur when the value type is the same, + and the key name has only changed names for clarity or cosmetic + purposes. + + When `overwrite` is `False` and the old key is found, an error is + raised. This generally occurs when the overall function of the old + and new keys has remained the same, but the value type has changed, + requiring the user to make changes to the config file. + + When `required` is `True` and neither the old or new keys are found, + an error is raised. When `required` is false and neither keys are + found, nothing happens. + """ + + old: str + new: str + overwrite: bool = False + required: bool = True + + +LEGACY_FIELDS: Dict[str, Tuple[LegacyField, ...]] = { + "Device": ( + LegacyField(old="nos", new="platform", overwrite=True), + LegacyField(old="network", new="group", overwrite=False, required=False), + ), + "Proxy": (LegacyField(old="nos", new="platform", overwrite=True),), } -def check_legacy_fields(model: str, **kwargs: Dict[str, Any]) -> Dict[str, Any]: +def check_legacy_fields(*, model: str, data: Dict[str, Any]) -> Dict[str, Any]: """Check for legacy fields prior to model initialization.""" if model in LEGACY_FIELDS: - for legacy_key, new_key in LEGACY_FIELDS[model]: - legacy_value = kwargs.pop(legacy_key, None) - new_value = kwargs.get(new_key) + for field in LEGACY_FIELDS[model]: + legacy_value = data.pop(field.old, None) + new_value = data.get(field.new) if legacy_value is not None and new_value is None: - log.warning( - "The {} field has been deprecated and will be removed in a future release. Use the '{}' field moving forward.", - f"{model}.{legacy_key}", - new_key, - ) - kwargs[new_key] = legacy_value - elif legacy_value is None and new_value is None: - raise ValueError(f"'{new_key}' is missing") - return kwargs + if field.overwrite: + log.warning( + ( + "The {!r} field has been deprecated and will be removed in a future release. " + "Use the {!r} field moving forward." + ), + f"{model}.{field.old}", + field.new, + ) + data[field.new] = legacy_value + else: + raise ValueError( + ( + "The {!r} field has been replaced with the {!r} field. " + "Please consult the documentation and/or changelog to determine the appropriate migration path." + ).format(f"{model}.{field.old}", field.new) + ) + elif legacy_value is None and new_value is None and field.required: + raise ValueError(f"'{field.new}' is missing") + return data diff --git a/hyperglass/plugins/tests/test_bgp_route_juniper.py b/hyperglass/plugins/tests/test_bgp_route_juniper.py index ef9aed9..1b7781c 100644 --- a/hyperglass/plugins/tests/test_bgp_route_juniper.py +++ b/hyperglass/plugins/tests/test_bgp_route_juniper.py @@ -17,7 +17,10 @@ from hyperglass.models.data.bgp_route import BGPRouteTable from .._builtin.bgp_route_juniper import BGPRoutePluginJuniper DEPENDS_KWARGS = { - "depends": ["hyperglass/external/tests/test_rpki.py::test_rpki"], + "depends": [ + "hyperglass/models/tests/test_util.py::test_check_legacy_fields", + "hyperglass/external/tests/test_rpki.py::test_rpki", + ], "scope": "session", } @@ -32,7 +35,7 @@ def _tester(sample: str): device = Device( name="Test Device", address="127.0.0.1", - network={"name": "Test Network", "display_name": "Test Network"}, + group="Test Network", credential={"username": "", "password": ""}, platform="juniper", structured_output=True, diff --git a/hyperglass/ui/components/form/queryLocation.tsx b/hyperglass/ui/components/form/queryLocation.tsx index 2063fc6..4050df3 100644 --- a/hyperglass/ui/components/form/queryLocation.tsx +++ b/hyperglass/ui/components/form/queryLocation.tsx @@ -6,18 +6,18 @@ import { Select } from '~/components'; import { useConfig, useColorValue } from '~/context'; import { useOpposingColor, useFormState } from '~/hooks'; -import type { Network, SingleOption, OptionGroup, FormData } from '~/types'; +import type { DeviceGroup, SingleOption, OptionGroup, FormData } from '~/types'; import type { TQuerySelectField, LocationCardProps } from './types'; -function buildOptions(networks: Network[]): OptionGroup[] { - return networks - .map(net => { - const label = net.displayName; - const options = net.locations +function buildOptions(devices: DeviceGroup[]): OptionGroup[] { + return devices + .map(group => { + const label = group.group; + const options = group.locations .map(loc => ({ label: loc.name, value: loc.id, - group: net.displayName, + group: loc.group, })) .sort((a, b) => (a.label < b.label ? -1 : a.label > b.label ? 1 : 0)); return { label, options }; @@ -115,14 +115,14 @@ const LocationCard = (props: LocationCardProps): JSX.Element => { export const QueryLocation = (props: TQuerySelectField): JSX.Element => { const { onChange, label } = props; - const { networks } = useConfig(); + const { devices } = useConfig(); const { formState: { errors }, } = useFormContext(); const selections = useFormState(s => s.selections); const setSelection = useFormState(s => s.setSelection); const { form, filtered } = useFormState(({ form, filtered }) => ({ form, filtered })); - const options = useMemo(() => buildOptions(networks), [networks]); + const options = useMemo(() => buildOptions(devices), [devices]); const element = useMemo(() => { const groups = options.length; const maxOptionsPerGroup = Math.max(...options.map(opt => opt.options.length)); diff --git a/hyperglass/ui/components/results/individual.tsx b/hyperglass/ui/components/results/individual.tsx index bd08047..3751638 100644 --- a/hyperglass/ui/components/results/individual.tsx +++ b/hyperglass/ui/components/results/individual.tsx @@ -7,6 +7,7 @@ import { chakra, HStack, Tooltip, + useToast, AccordionItem, AccordionPanel, AccordionButton, @@ -44,6 +45,7 @@ const _Result: React.ForwardRefRenderFunction = ( ref, ) => { const { index, queryLocation } = props; + const toast = useToast(); const { web, cache, messages } = useConfig(); const { index: indices, setIndex } = useAccordionContext(); const getDevice = useDevice(); @@ -66,7 +68,9 @@ const _Result: React.ForwardRefRenderFunction = ( }, { onSuccess(data) { - addResponse(device.id, data); + if (device !== null) { + addResponse(device.id, data); + } }, onError(error) { console.error(error); @@ -150,6 +154,21 @@ const _Result: React.ForwardRefRenderFunction = ( } } }, [data, index, indices, isLoading, isError, setIndex]); + + if (device === null) { + const id = `toast-queryLocation-${index}-${queryLocation}`; + if (!toast.isActive(id)) { + toast({ + id, + title: messages.general, + description: `Configuration for device with ID '${queryLocation}' not found.`, + status: 'error', + isClosable: true, + }); + } + return <>; + } + return ( Device; +) => Device | null; export type UseStrfArgs = { [k: string]: unknown } | string; diff --git a/hyperglass/ui/hooks/useDevice.ts b/hyperglass/ui/hooks/useDevice.ts index 7190726..ed3dad9 100644 --- a/hyperglass/ui/hooks/useDevice.ts +++ b/hyperglass/ui/hooks/useDevice.ts @@ -2,19 +2,19 @@ import { useCallback, useMemo } from 'react'; import { useConfig } from '~/context'; import type { Device } from '~/types'; -import type { TUseDevice } from './types'; +import type { UseDevice } from './types'; /** * Get a device's configuration from the global configuration context based on its name. */ -export function useDevice(): TUseDevice { - const { networks } = useConfig(); +export function useDevice(): UseDevice { + const { devices } = useConfig(); - const devices = useMemo(() => networks.map(n => n.locations).flat(), [networks]); + const locations = useMemo(() => devices.map(group => group.locations).flat(), [devices]); - function getDevice(id: string): Device { - return devices.filter(dev => dev.id === id)[0]; + function getDevice(id: string): Device | null { + return locations.find(device => device.id === id) ?? null; } - return useCallback(getDevice, [devices]); + return useCallback(getDevice, [locations]); } diff --git a/hyperglass/ui/hooks/useFormState.ts b/hyperglass/ui/hooks/useFormState.ts index 88a9318..d1fe941 100644 --- a/hyperglass/ui/hooks/useFormState.ts +++ b/hyperglass/ui/hooks/useFormState.ts @@ -8,7 +8,7 @@ import { all, andJoin, dedupObjectArray, withDev } from '~/util'; import type { StateCreator } from 'zustand'; import type { UseFormSetError, UseFormClearErrors } from 'react-hook-form'; import type { SingleOption, Directive, FormData, Text } from '~/types'; -import type { TUseDevice } from './types'; +import type { UseDevice } from './types'; type FormStatus = 'form' | 'results'; @@ -67,7 +67,7 @@ interface FormStateType { extra: { setError: UseFormSetError; clearErrors: UseFormClearErrors; - getDevice: TUseDevice; + getDevice: UseDevice; text: Text; }, ): void; @@ -129,7 +129,7 @@ const formState: StateCreator = (set, get) => ({ extra: { setError: UseFormSetError; clearErrors: UseFormClearErrors; - getDevice: TUseDevice; + getDevice: UseDevice; text: Text; }, ): void { @@ -144,15 +144,17 @@ const formState: StateCreator = (set, get) => ({ for (const loc of locations) { const device = getDevice(loc); - locationNames.push(device.name); - allDevices.push(device); - const groups = new Set(); - for (const directive of device.directives) { - for (const group of directive.groups) { - groups.add(group); + if (device !== null) { + locationNames.push(device.name); + allDevices.push(device); + const groups = new Set(); + for (const directive of device.directives) { + for (const group of directive.groups) { + groups.add(group); + } } + allGroups.push(Array.from(groups)); } - allGroups.push(Array.from(groups)); } const intersecting = intersectionWith(...allGroups, isEqual); diff --git a/hyperglass/ui/types/config.ts b/hyperglass/ui/types/config.ts index b696d47..adae2f8 100644 --- a/hyperglass/ui/types/config.ts +++ b/hyperglass/ui/types/config.ts @@ -97,36 +97,6 @@ interface _Web { theme: _ThemeConfig; } -// export interface Query { -// name: string; -// enable: boolean; -// display_name: string; -// } - -// export interface BGPCommunity { -// community: string; -// display_name: string; -// description: string; -// } - -// export interface QueryBGPRoute extends Query {} -// export interface QueryBGPASPath extends Query {} -// export interface QueryPing extends Query {} -// export interface QueryTraceroute extends Query {} -// export interface QueryBGPCommunity extends Query { -// mode: 'input' | 'select'; -// communities: BGPCommunity[]; -// } - -// export interface Queries { -// bgp_route: QueryBGPRoute; -// bgp_community: QueryBGPCommunity; -// bgp_aspath: QueryBGPASPath; -// ping: QueryPing; -// traceroute: QueryTraceroute; -// list: Query[]; -// } - type _DirectiveBase = { id: string; name: string; @@ -151,15 +121,10 @@ type _Directive = _DirectiveBase | _DirectiveSelect; interface _Device { id: string; name: string; - network: string; + group: string; directives: _Directive[]; } -interface _Network { - display_name: string; - locations: _Device[]; -} - interface _QueryContent { content: string; enable: boolean; @@ -184,13 +149,16 @@ interface _Cache { type _Config = _ConfigDeep & _ConfigShallow; +interface _DeviceGroup { + group: string; + locations: _Device[]; +} + interface _ConfigDeep { cache: _Cache; web: _Web; messages: _Messages; - // queries: Queries; - devices: _Device[]; - networks: _Network[]; + devices: _DeviceGroup[]; content: _Content; } @@ -225,8 +193,8 @@ export type Config = CamelCasedPropertiesDeep<_ConfigDeep> & CamelCasedPropertie export type ThemeConfig = CamelCasedProperties<_ThemeConfig>; export type Content = CamelCasedProperties<_Content>; export type QueryContent = CamelCasedPropertiesDeep<_QueryContent>; -export type Network = CamelCasedPropertiesDeep<_Network>; export type Device = CamelCasedPropertiesDeep<_Device>; +export type DeviceGroup = CamelCasedPropertiesDeep<_DeviceGroup>; export type Directive = CamelCasedPropertiesDeep<_Directive>; export type DirectiveSelect = CamelCasedPropertiesDeep<_DirectiveSelect>; export type DirectiveOption = CamelCasedPropertiesDeep<_DirectiveOption>;